From: Linus Torvalds <torvalds@linux-foundation.org> To: Eric Biggers <ebiggers@kernel.org> Cc: Suren Baghdasaryan <surenb@google.com>, Johannes Weiner <hannes@cmpxchg.org>, Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>, Juri Lelli <juri.lelli@redhat.com>, Vincent Guittot <vincent.guittot@linaro.org>, Dietmar Eggemann <dietmar.eggemann@arm.com>, Steven Rostedt <rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>, Daniel Bristot de Oliveira <bristot@redhat.com>, Jonathan Corbet <corbet@lwn.net>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Cgroups <cgroups@vger.kernel.org>, stable <stable@vger.kernel.org>, Android Kernel Team <kernel-team@android.com>, syzbot <syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com> Subject: Re: [PATCH v2 1/1] psi: Fix uaf issue when psi trigger is destroyed while being polled Date: Tue, 11 Jan 2022 11:11:32 -0800 [thread overview] Message-ID: <CAHk-=wgwb6pJjvHYmOMT-yp5RYvw0pbv810Wcxdm5S7dWc-s0g@mail.gmail.com> (raw) In-Reply-To: <Yd3RClhoz24rrU04@sol.localdomain> On Tue, Jan 11, 2022 at 10:48 AM Eric Biggers <ebiggers@kernel.org> wrote: > > The write here needs to use smp_store_release(), since it is paired with the > concurrent READ_ONCE() in psi_trigger_poll(). A smp_store_release() doesn't make sense pairing with a READ_ONCE(). Any memory ordering that the smp_store_release() does on the writing side is entirely irrelevant, since the READ_ONCE() doesn't imply any ordering on the reading side. Ordering one but not the other is nonsensical. So the proper pattern is to use a WRITE_ONCE() to pair with a READ_ONCE() (when you don't care about memory ordering, or you handle it explicitly), or a smp_load_acquire() with a smp_store_release() (in which case writes before the smp_store_release() on the writing side will be ordered wrt accesses after smp_load_acquire() on the reading side). Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings) But in this case, I do think that since there's some setup involved with the trigger pointer, the proper serialization is to use smp_store_release() to set the pointer, and then smp_load_acquire() on the reading side. Or just use the RCU primitives - they are even better optimized, and handle exactly that case, and can be more efficient on some architectures if release->acquire isn't already cheap. That said, we've pretty much always accepted that normal word writes are not going to tear, so we *have* also accepted just - do any normal store of a value on the write side - do a READ_ONCE() on the reading side where the reading side doesn't actually care *what* value it gets, it only cares that the value it gets is *stable* (ie no compiler reloads that might show up as two different values on the reading side). Of course, that has the same issue as WRITE_ONCE/READ_ONCE - you need to worry about memory ordering separately. > > + seq->private = new; > > Likewise here. Yeah, same deal, except here you can't even use the RCU ones, because 'seq->private' isn't annotated for RCU. Or you'd do the casting, of course. Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> To: Eric Biggers <ebiggers-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Zefan Li <lizefan.x-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org>, Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Juri Lelli <juri.lelli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Vincent Guittot <vincent.guittot-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Dietmar Eggemann <dietmar.eggemann-5wv7dgnIgG8@public.gmane.org>, Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>, Ben Segall <bsegall-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Mel Gorman <mgorman-l3A5Bk7waGM@public.gmane.org>, Daniel Bristot de Oliveira <bristot-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>, "open list:DOCUMENTATION" <linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux Kernel Mailing List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Cgroups <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>, syzbot <s> Subject: Re: [PATCH v2 1/1] psi: Fix uaf issue when psi trigger is destroyed while being polled Date: Tue, 11 Jan 2022 11:11:32 -0800 [thread overview] Message-ID: <CAHk-=wgwb6pJjvHYmOMT-yp5RYvw0pbv810Wcxdm5S7dWc-s0g@mail.gmail.com> (raw) In-Reply-To: <Yd3RClhoz24rrU04-8Kz7deQLoS0mYvmMESoHnA@public.gmane.org> On Tue, Jan 11, 2022 at 10:48 AM Eric Biggers <ebiggers-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > > The write here needs to use smp_store_release(), since it is paired with the > concurrent READ_ONCE() in psi_trigger_poll(). A smp_store_release() doesn't make sense pairing with a READ_ONCE(). Any memory ordering that the smp_store_release() does on the writing side is entirely irrelevant, since the READ_ONCE() doesn't imply any ordering on the reading side. Ordering one but not the other is nonsensical. So the proper pattern is to use a WRITE_ONCE() to pair with a READ_ONCE() (when you don't care about memory ordering, or you handle it explicitly), or a smp_load_acquire() with a smp_store_release() (in which case writes before the smp_store_release() on the writing side will be ordered wrt accesses after smp_load_acquire() on the reading side). Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings) But in this case, I do think that since there's some setup involved with the trigger pointer, the proper serialization is to use smp_store_release() to set the pointer, and then smp_load_acquire() on the reading side. Or just use the RCU primitives - they are even better optimized, and handle exactly that case, and can be more efficient on some architectures if release->acquire isn't already cheap. That said, we've pretty much always accepted that normal word writes are not going to tear, so we *have* also accepted just - do any normal store of a value on the write side - do a READ_ONCE() on the reading side where the reading side doesn't actually care *what* value it gets, it only cares that the value it gets is *stable* (ie no compiler reloads that might show up as two different values on the reading side). Of course, that has the same issue as WRITE_ONCE/READ_ONCE - you need to worry about memory ordering separately. > > + seq->private = new; > > Likewise here. Yeah, same deal, except here you can't even use the RCU ones, because 'seq->private' isn't annotated for RCU. Or you'd do the casting, of course. Linus
next prev parent reply other threads:[~2022-01-11 19:20 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-11 7:12 [PATCH v2 1/1] psi: Fix uaf issue when psi trigger is destroyed while being polled Suren Baghdasaryan 2022-01-11 18:48 ` Eric Biggers 2022-01-11 19:11 ` Linus Torvalds [this message] 2022-01-11 19:11 ` Linus Torvalds 2022-01-11 19:26 ` Suren Baghdasaryan 2022-01-11 19:26 ` Suren Baghdasaryan 2022-01-11 19:34 ` Linus Torvalds 2022-01-11 19:34 ` Linus Torvalds 2022-01-11 19:41 ` Eric Biggers 2022-01-11 19:41 ` Eric Biggers 2022-01-11 19:45 ` Suren Baghdasaryan 2022-01-11 19:45 ` Suren Baghdasaryan 2022-01-11 20:14 ` Linus Torvalds 2022-01-11 20:14 ` Linus Torvalds 2022-01-11 23:37 ` Suren Baghdasaryan 2022-01-11 23:37 ` Suren Baghdasaryan 2022-01-12 11:06 ` Peter Zijlstra 2022-01-12 11:06 ` Peter Zijlstra 2022-01-18 6:58 ` Herbert Xu 2022-01-12 14:37 ` Johannes Weiner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAHk-=wgwb6pJjvHYmOMT-yp5RYvw0pbv810Wcxdm5S7dWc-s0g@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --cc=bristot@redhat.com \ --cc=bsegall@google.com \ --cc=cgroups@vger.kernel.org \ --cc=corbet@lwn.net \ --cc=dietmar.eggemann@arm.com \ --cc=ebiggers@kernel.org \ --cc=hannes@cmpxchg.org \ --cc=juri.lelli@redhat.com \ --cc=kernel-team@android.com \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan.x@bytedance.com \ --cc=mgorman@suse.de \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=stable@vger.kernel.org \ --cc=surenb@google.com \ --cc=syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com \ --cc=tj@kernel.org \ --cc=vincent.guittot@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.