From: Suren Baghdasaryan <surenb@google.com> To: Eric Biggers <ebiggers@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org>, 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:45:50 -0800 [thread overview] Message-ID: <CAJuCfpHq+YuvGxUN_6T0Hhit9xqrHptEURUYA9yoQ=a4Mxm=ug@mail.gmail.com> (raw) In-Reply-To: <Yd3dZklleDnJCQ46@gmail.com> On Tue, Jan 11, 2022 at 11:41 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jan 11, 2022 at 11:11:32AM -0800, Linus Torvalds wrote: > > 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. > > > > This is yet another case of "one time init". There have been long discussions > on this topic before: > * https://lore.kernel.org/linux-fsdevel/20200713033330.205104-1-ebiggers@kernel.org/T/#u > * https://lore.kernel.org/lkml/20200916233042.51634-1-ebiggers@kernel.org/T/#u > * https://lwn.net/Articles/827180/ > > I even attempted to document the best practices: > * https://lore.kernel.org/linux-fsdevel/20200717044427.68747-1-ebiggers@kernel.org/T/#u > > However, no one could agree on whether READ_ONCE() or smp_load_acquire() should > be used. smp_load_acquire() is always correct, so it remains my preference. > However, READ_ONCE() is correct in some cases, and some people (including the > primary LKMM maintainer) insist that it be used in all such cases, as well as in > rcu_dereference() even though this places difficult-to-understand constraints on > how rcu_dereference() can be used. > > My preference is that smp_load_acquire() be used. But be aware that this risks > the READ_ONCE() people coming out of the woodwork and arguing for READ_ONCE(). I like my chances here (I believe we do need memory ordering in this case). I'll post a fix with smp_load_acquire/smp_store_release shortly after I run my tests. Thanks for the guidance! > > - Eric
WARNING: multiple messages have this Message-ID (diff)
From: Suren Baghdasaryan <surenb@google.com> To: Eric Biggers <ebiggers@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org>, 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>, 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:45:50 -0800 [thread overview] Message-ID: <CAJuCfpHq+YuvGxUN_6T0Hhit9xqrHptEURUYA9yoQ=a4Mxm=ug@mail.gmail.com> (raw) In-Reply-To: <Yd3dZklleDnJCQ46@gmail.com> On Tue, Jan 11, 2022 at 11:41 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jan 11, 2022 at 11:11:32AM -0800, Linus Torvalds wrote: > > 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. > > > > This is yet another case of "one time init". There have been long discussions > on this topic before: > * https://lore.kernel.org/linux-fsdevel/20200713033330.205104-1-ebiggers@kernel.org/T/#u > * https://lore.kernel.org/lkml/20200916233042.51634-1-ebiggers@kernel.org/T/#u > * https://lwn.net/Articles/827180/ > > I even attempted to document the best practices: > * https://lore.kernel.org/linux-fsdevel/20200717044427.68747-1-ebiggers@kernel.org/T/#u > > However, no one could agree on whether READ_ONCE() or smp_load_acquire() should > be used. smp_load_acquire() is always correct, so it remains my preference. > However, READ_ONCE() is correct in some cases, and some people (including the > primary LKMM maintainer) insist that it be used in all such cases, as well as in > rcu_dereference() even though this places difficult-to-understand constraints on > how rcu_dereference() can be used. > > My preference is that smp_load_acquire() be used. But be aware that this risks > the READ_ONCE() people coming out of the woodwork and arguing for READ_ONCE(). I like my chances here (I believe we do need memory ordering in this case). I'll post a fix with smp_load_acquire/smp_store_release shortly after I run my tests. Thanks for the guidance! > > - Eric
next prev parent reply other threads:[~2022-01-11 19:46 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 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 [this message] 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='CAJuCfpHq+YuvGxUN_6T0Hhit9xqrHptEURUYA9yoQ=a4Mxm=ug@mail.gmail.com' \ --to=surenb@google.com \ --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=syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com \ --cc=tj@kernel.org \ --cc=torvalds@linux-foundation.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.