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

[Liang, Kan]

> > > > +	/*
> > > > +	 * 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