Re: perf_buffer.event_list is not RCU-safe?

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Apr 06, 2021 at 07:43:53PM +0200, Oleg Nesterov wrote:
> On 04/06, Oleg Nesterov wrote:
> >
> > perf_mmap_close() was added by 9bb5d40cd93c9 ("perf: Fix mmap() accounting hole")
> 
> I meant perf_mmap_close() -> put_event()
> 
> > and this commit doesn't look right anyway
> 
> It seems there is another problem or I am totally confused. I do not
> understand why can we use list_for_each_entry_rcu(event, rb->event_list)
> if this can race with perf_event_set_output(event) which can move "event"
> to another list, in this case list_for_each_entry_rcu() can loop forever.
> 
> perf_mmap_close() even mentions this race and restarts the iteration to
> avoid it but I don't think this is enough,
> 
> 	rcu_read_lock();
> 	list_for_each_entry_rcu(event, &rb->event_list, rb_entry) {
> 		if (!atomic_long_inc_not_zero(&event->refcount)) {
> 			/*
> 			 * This event is en-route to free_event() which will
> 			 * detach it and remove it from the list.
> 			 */
> 			continue;
> 		}
> 
> just suppose that "this event" is moved to another list first and after
> that it goes away so that atomic_long_inc_not_zero() fails; in this case
> the next iteration will play with event->rb_entry.next, and this is not
> necessarily "struct perf_event", it can can be "list_head event_list".

We observe an RCU GP in ring_buffer_attach(), between detach and attach,
no?

Normally, when we attach to a rb for the first time, or when we remove
it first, no GP is required and everything is fine. But when we remove
it and then attach it again to another rb, we must observe a GP because
of that list_rcu, agreed?

The cond_synchronize_rcu() in ring_buffer_attach() should capture
exactly that case.