Messages in this thread | | | Subject | Re: [PATCH] perf/x86: Consider pinned events for group validation | From | "Liang, Kan" <> | Date | Tue, 20 Aug 2019 13:13:36 -0400 |
| |
>>>> + /* >>>> + * 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().
Thanks, Kan
| |