| Message ID | 20241004182734.1761555-3-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sched+mm: Track lazy active mm existence with hazard pointers | expand |
On Fri, Oct 4, 2024 at 2:29 PM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > Refer to ptr_eq() in the rcu_dereference() documentation. > > ptr_eq() is a mechanism that preserves address dependencies when > comparing pointers, and should be favored when comparing a pointer > obtained from rcu_dereference() against another pointer. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Acked-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Will Deacon <will@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: John Stultz <jstultz@google.com> > Cc: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Boqun Feng <boqun.feng@gmail.com> > Cc: Frederic Weisbecker <frederic@kernel.org> > Cc: Joel Fernandes <joel@joelfernandes.org> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> thanks, - Joel
From: Mathieu Desnoyers > Sent: 04 October 2024 19:28 > > Refer to ptr_eq() in the rcu_dereference() documentation. > > ptr_eq() is a mechanism that preserves address dependencies when > comparing pointers, and should be favored when comparing a pointer > obtained from rcu_dereference() against another pointer. Why does this ever really matter for rcu? The check just ensure that any speculative load uses a specific one of the pointers when they are different. This can only matter if you care about the side effects of the speculative load. But rcu is all about (things like) lockless list following. So you need to wait until it is impossible for another execution context to have a reference to some memory before actually completely invalidating it (ie kfree()). And that 50 line comment is pointless. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, Oct 06, 2024 at 07:52:49PM +0000, David Laight wrote: > From: Mathieu Desnoyers > > Sent: 04 October 2024 19:28 > > > > Refer to ptr_eq() in the rcu_dereference() documentation. > > > > ptr_eq() is a mechanism that preserves address dependencies when > > comparing pointers, and should be favored when comparing a pointer > > obtained from rcu_dereference() against another pointer. > > Why does this ever really matter for rcu? > > The check just ensure that any speculative load uses > a specific one of the pointers when they are different. > This can only matter if you care about the side effects > of the speculative load. It can matter when there are static variables used during OOM. The code must check the pointer against the addresses of the static variables to work out how to free them, which can enable the compiler to do specialization optimizations that destroy the address dependencies. Which is OK if those static variables are compile-time initialized or some such. But not if they get new values each time they go into the list, as this can result in the reader seeing pre-initialization garbage. An admittedly rare but very real use case. Thanx, Paul > But rcu is all about (things like) lockless list following. > So you need to wait until it is impossible for another > execution context to have a reference to some memory > before actually completely invalidating it (ie kfree()). > > And that 50 line comment is pointless. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
On 2024-10-06 19:52:49 [+0000], David Laight wrote: > From: Mathieu Desnoyers > > Sent: 04 October 2024 19:28 > > > > Refer to ptr_eq() in the rcu_dereference() documentation. > > > > ptr_eq() is a mechanism that preserves address dependencies when > > comparing pointers, and should be favored when comparing a pointer > > obtained from rcu_dereference() against another pointer. > > Why does this ever really matter for rcu? > > The check just ensure that any speculative load uses > a specific one of the pointers when they are different. > This can only matter if you care about the side effects > of the speculative load. > > But rcu is all about (things like) lockless list following. > So you need to wait until it is impossible for another > execution context to have a reference to some memory > before actually completely invalidating it (ie kfree()). Not always. Non-RCU could would have locking with a barrier to ensure a reload. RCU would not have the barrier. Assuming the pointer, points to a struct, the compiler could load an element from the first pointer and keeping it after it ensured the pointer are equal. However based on the logic, the content could have changed and the compiler would not load the new value but keep the previous one. > David Sebastian
diff --git a/Documentation/RCU/rcu_dereference.rst b/Documentation/RCU/rcu_dereference.rst index 2524dcdadde2..de6175bf430f 100644 --- a/Documentation/RCU/rcu_dereference.rst +++ b/Documentation/RCU/rcu_dereference.rst @@ -104,11 +104,12 @@ readers working properly: after such branches, but can speculate loads, which can again result in misordering bugs. -- Be very careful about comparing pointers obtained from - rcu_dereference() against non-NULL values. As Linus Torvalds - explained, if the two pointers are equal, the compiler could - substitute the pointer you are comparing against for the pointer - obtained from rcu_dereference(). For example:: +- Use operations that preserve address dependencies (such as + "ptr_eq()") to compare pointers obtained from rcu_dereference() + against non-NULL pointers. As Linus Torvalds explained, if the + two pointers are equal, the compiler could substitute the + pointer you are comparing against for the pointer obtained from + rcu_dereference(). For example:: p = rcu_dereference(gp); if (p == &default_struct) @@ -125,6 +126,29 @@ readers working properly: On ARM and Power hardware, the load from "default_struct.a" can now be speculated, such that it might happen before the rcu_dereference(). This could result in bugs due to misordering. + Performing the comparison with "ptr_eq()" ensures the compiler + does not perform such transformation. + + If the comparison is against another pointer, the compiler is + allowed to use either pointer for the following accesses, which + loses the address dependency and allows weakly-ordered + architectures such as ARM and PowerPC to speculate the + address-dependent load before rcu_dereference(). For example:: + + p1 = READ_ONCE(gp); + p2 = rcu_dereference(gp); + if (p1 == p2) /* BUGGY!!! */ + do_default(p2->a); + + The compiler can use p1->a rather than p2->a, destroying the + address dependency. Performing the comparison with "ptr_eq()" + ensures the compiler preserves the address dependencies. + Corrected code:: + + p1 = READ_ONCE(gp); + p2 = rcu_dereference(gp); + if (ptr_eq(p1, p2)) + do_default(p2->a); However, comparisons are OK in the following cases: @@ -204,6 +228,10 @@ readers working properly: comparison will provide exactly the information that the compiler needs to deduce the value of the pointer. + When in doubt, use operations that preserve address dependencies + (such as "ptr_eq()") to compare pointers obtained from + rcu_dereference() against non-NULL pointers. + - Disable any value-speculation optimizations that your compiler might provide, especially if you are making use of feedback-based optimizations that take data collected from prior runs. Such