[RFC,v2,2/4] Documentation: RCU: Refer to ptr_eq()

Commit Message

Mathieu Desnoyers Oct. 4, 2024, 6:27 p.m. UTC

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 <[e-mail]>
Acked-by: Alan Stern <[e-mail]>
Acked-by: Paul E. McKenney <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Sebastian Andrzej Siewior <[e-mail]>
Cc: "Paul E. McKenney" <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Boqun Feng <[e-mail]>
Cc: Alan Stern <[e-mail]>
Cc: John Stultz <[e-mail]>
Cc: Neeraj Upadhyay <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
Cc: Boqun Feng <[e-mail]>
Cc: Frederic Weisbecker <[e-mail]>
Cc: Joel Fernandes <[e-mail]>
Cc: Josh Triplett <[e-mail]>
Cc: Uladzislau Rezki <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Lai Jiangshan <[e-mail]>
Cc: Zqiang <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Waiman Long <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Vlastimil Babka <[e-mail]>
Cc: [e-mail]
Cc: Mateusz Guzik <[e-mail]>
Cc: Gary Guo <[e-mail]>
Cc: Jonas Oberhauser <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: Nikita Popov <[e-mail]>
Cc: [e-mail]
---
Changes since v0:
- Include feedback from Alan Stern.

Changes since v1:
- Include feedback from Paul E. McKenney.
---
 Documentation/RCU/rcu_dereference.rst | 38 +++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

Joel Fernandes Oct. 4, 2024, 9:15 p.m. UTC | #1

On Fri, Oct 4, 2024 at 2:29 PM Mathieu Desnoyers
<[e-mail]> 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 <[e-mail]>
> Acked-by: Alan Stern <[e-mail]>
> Acked-by: Paul E. McKenney <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Sebastian Andrzej Siewior <[e-mail]>
> Cc: "Paul E. McKenney" <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> Cc: Boqun Feng <[e-mail]>
> Cc: Alan Stern <[e-mail]>
> Cc: John Stultz <[e-mail]>
> Cc: Neeraj Upadhyay <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>
> Cc: Boqun Feng <[e-mail]>
> Cc: Frederic Weisbecker <[e-mail]>
> Cc: Joel Fernandes <[e-mail]>

Reviewed-by: Joel Fernandes (Google) <[e-mail]>

thanks,

 - Joel

David Laight Oct. 6, 2024, 7:52 p.m. UTC | #2

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)

Paul E. McKenney Oct. 6, 2024, 8:39 p.m. UTC | #3

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)
>

Sebastian Andrzej Siewior Oct. 7, 2024, 11:01 a.m. UTC | #4

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

Patch

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