linux-kernel - Re: perf_buffer.event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Apr 2021 09:50:52 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Hillf Danton <hdanton@...a.com>, Song Liu <songliubraving@...com>,
        Namhyung Kim <namhyung@...nel.org>,
        syzbot <syzbot+b804f902bbb6bcf290cb@...kaller.appspotmail.com>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: perf_buffer.event_list is not RCU-safe?

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.