Re: [PATCH] perf/x86: Consider pinned events for group validation

From: Liang, Kan
Date: Tue Aug 20 2019 - 13:13:40 EST

+ /*
+ * The new group must can be scheduled
+ * together with current pinned events.
+ * Otherwise, it will never get a chance
+ * to be scheduled later.

That's wrapped short; also I don't think it is sufficient; what if you
happen to have a pinned event on CPU1 (and not others) and happen to run
validation for a new CPU1 event on CPUn ?

The patch doesn't support this case.

Which makes the whole thing even more random.

Maybe we can use the cpuc on event->cpu. That could help a little here.
cpuc = per_cpu_ptr(&cpu_hw_events, event->cpu >= 0 ? event->cpu : raw_smp_processor_id());

It is mentioned in the description.
The patch doesn't intend to catch all possible cases that cannot be
scheduled. I think it's impossible to catch all cases.
We only want to improve the validate_group() a little bit to catch some
common cases, e.g. NMI watchdog interacting with group.

Also; per that same; it is broken, you're accessing the cpu-local cpuc
without serialization.

Do you mean accessing all cpuc serially?
We only check the cpuc on current CPU here. It doesn't intend to access
other cpuc.

There's nothing preventing the cpuc you're looking at changing while
you're looking at it. Heck, afaict it is possible to UaF here. Nothing
prevents the events you're looking at from going away and getting freed.

You are right.
I think we can add a lock to prevent the event_list[] in x86_pmu_add() and x86_pmu_del().