diff mbox series

[6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls

Message ID 20230717180317.1097590-6-paulmck@kernel.org (mailing list archive)
State Accepted
Commit f4404d575c11b6401d10d0ea21dec5cd937720d7
Headers show
Series Miscellaneous fixes for v6.6 | expand

Commit Message

Paul E. McKenney July 17, 2023, 6:03 p.m. UTC 
From: Alan Huang <mmpgouride@gmail.com>

When the objects managed by rculist_nulls are allocated with
SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
even though it is just now being added, which means the modification of
->next is visible to readers.  This patch therefore uses WRITE_ONCE()
for assignments to ->next.

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 include/linux/rculist_nulls.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Joel Fernandes July 18, 2023, 1:49 p.m. UTC | #1 
On 7/17/23 14:03, Paul E. McKenney wrote:
> From: Alan Huang <mmpgouride@gmail.com>
> 
> When the objects managed by rculist_nulls are allocated with
> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> even though it is just now being added, which means the modification of
> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
> for assignments to ->next.
> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)

But incremental progress and all, so this LGTM:
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

  - Joel


> ---
>   include/linux/rculist_nulls.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index ba4c00dd8005..89186c499dd4 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>   {
>   	struct hlist_nulls_node *first = h->first;
>   
> -	n->next = first;
> +	WRITE_ONCE(n->next, first);
>   	WRITE_ONCE(n->pprev, &h->first);
>   	rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
>   	if (!is_a_nulls(first))
> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>   		last = i;
>   
>   	if (last) {
> -		n->next = last->next;
> +		WRITE_ONCE(n->next, last->next);
>   		n->pprev = &last->next;
>   		rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>   	} else {
Alan Huang July 18, 2023, 2:48 p.m. UTC | #2 
> 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> 
> On 7/17/23 14:03, Paul E. McKenney wrote:
>> From: Alan Huang <mmpgouride@gmail.com>
>> When the objects managed by rculist_nulls are allocated with
>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>> even though it is just now being added, which means the modification of
>> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
>> for assignments to ->next.
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)

Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.

> 
> But incremental progress and all, so this LGTM:
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> thanks,
> 
> - Joel
> 
> 
>> ---
>>  include/linux/rculist_nulls.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
>> index ba4c00dd8005..89186c499dd4 100644
>> --- a/include/linux/rculist_nulls.h
>> +++ b/include/linux/rculist_nulls.h
>> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>>  {
>>   struct hlist_nulls_node *first = h->first;
>>  - n->next = first;
>> + WRITE_ONCE(n->next, first);
>>   WRITE_ONCE(n->pprev, &h->first);
>>   rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
>>   if (!is_a_nulls(first))
>> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>>   last = i;
>>     if (last) {
>> - n->next = last->next;
>> + WRITE_ONCE(n->next, last->next);
>>   n->pprev = &last->next;
>>   rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>>   } else {
>
Paul E. McKenney July 18, 2023, 6:32 p.m. UTC | #3 
On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> 
> > 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> > 
> > On 7/17/23 14:03, Paul E. McKenney wrote:
> >> From: Alan Huang <mmpgouride@gmail.com>
> >> When the objects managed by rculist_nulls are allocated with
> >> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> >> even though it is just now being added, which means the modification of
> >> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
> >> for assignments to ->next.
> >> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> >> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
> 
> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.

It turns out that different traversal synchronization designs want
different pointers using WRITE_ONCE().

> > But incremental progress and all, so this LGTM:
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

I will apply all four on my next rebase, thank you!

							Thanx, Paul

> > thanks,
> > 
> > - Joel
> > 
> > 
> >> ---
> >>  include/linux/rculist_nulls.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> >> index ba4c00dd8005..89186c499dd4 100644
> >> --- a/include/linux/rculist_nulls.h
> >> +++ b/include/linux/rculist_nulls.h
> >> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> >>  {
> >>   struct hlist_nulls_node *first = h->first;
> >>  - n->next = first;
> >> + WRITE_ONCE(n->next, first);
> >>   WRITE_ONCE(n->pprev, &h->first);
> >>   rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> >>   if (!is_a_nulls(first))
> >> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> >>   last = i;
> >>     if (last) {
> >> - n->next = last->next;
> >> + WRITE_ONCE(n->next, last->next);
> >>   n->pprev = &last->next;
> >>   rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> >>   } else {
> > 
>
Joel Fernandes July 19, 2023, 1:48 a.m. UTC | #4 
On 7/18/23 14:32, Paul E. McKenney wrote:
> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
>>
>>> 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
>>>
>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>> From: Alan Huang <mmpgouride@gmail.com>
>>>> When the objects managed by rculist_nulls are allocated with
>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>>>> even though it is just now being added, which means the modification of
>>>> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
>>>> for assignments to ->next.
>>>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>
>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
>>
>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
> 
> It turns out that different traversal synchronization designs want
> different pointers using WRITE_ONCE().

Thank you Alan and Paul,

Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it 
can be removed?

  - Joel
Paul E. McKenney July 19, 2023, 6:20 p.m. UTC | #5 
On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
> 
> 
> On 7/18/23 14:32, Paul E. McKenney wrote:
> > On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> > > 
> > > > 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> > > > 
> > > > On 7/17/23 14:03, Paul E. McKenney wrote:
> > > > > From: Alan Huang <mmpgouride@gmail.com>
> > > > > When the objects managed by rculist_nulls are allocated with
> > > > > SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> > > > > even though it is just now being added, which means the modification of
> > > > > ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
> > > > > for assignments to ->next.
> > > > > Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > 
> > > > Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
> > > 
> > > Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
> > 
> > It turns out that different traversal synchronization designs want
> > different pointers using WRITE_ONCE().
> 
> Thank you Alan and Paul,
> 
> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
> be removed?

Either that or the people who removed uses injected bugs...

But if this one really does go away, do we need ->pprev to be
protected by _ONCE()?

							Thanx, Paul
Alan Huang July 19, 2023, 7:17 p.m. UTC | #6 
> 2023年7月20日 02:20，Paul E. McKenney <paulmck@kernel.org> 写道：
> 
> On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
>> 
>> 
>> On 7/18/23 14:32, Paul E. McKenney wrote:
>>> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
>>>> 
>>>>> 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
>>>>> 
>>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
>>>>>> From: Alan Huang <mmpgouride@gmail.com>
>>>>>> When the objects managed by rculist_nulls are allocated with
>>>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
>>>>>> even though it is just now being added, which means the modification of
>>>>>> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
>>>>>> for assignments to ->next.
>>>>>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>>>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>>> 
>>>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
>>>> 
>>>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
>>> 
>>> It turns out that different traversal synchronization designs want
>>> different pointers using WRITE_ONCE().
>> 
>> Thank you Alan and Paul,
>> 
>> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
>> be removed?
> 
> Either that or the people who removed uses injected bugs...

It has never been used.

That said, the data race has been there almost for four years.

And the network people use sk_unhashed() for both hlist_node and hlist_nulls_node.
So, I plan to use hlist_unhashed_lockless() in sk_unhashed(), that will be one of my future patches.

> 
> But if this one really does go away, do we need ->pprev to be
> protected by _ONCE()?

The ->pprev thing is what I’m currently working on. :)

> 
> Thanx, Paul
Paul E. McKenney July 19, 2023, 8:02 p.m. UTC | #7 
On Thu, Jul 20, 2023 at 03:17:58AM +0800, Alan Huang wrote:
> 
> > 2023年7月20日 02:20，Paul E. McKenney <paulmck@kernel.org> 写道：
> > 
> > On Tue, Jul 18, 2023 at 09:48:59PM -0400, Joel Fernandes wrote:
> >> 
> >> 
> >> On 7/18/23 14:32, Paul E. McKenney wrote:
> >>> On Tue, Jul 18, 2023 at 10:48:07PM +0800, Alan Huang wrote:
> >>>> 
> >>>>> 2023年7月18日 21:49，Joel Fernandes <joel@joelfernandes.org> 写道：
> >>>>> 
> >>>>> On 7/17/23 14:03, Paul E. McKenney wrote:
> >>>>>> From: Alan Huang <mmpgouride@gmail.com>
> >>>>>> When the objects managed by rculist_nulls are allocated with
> >>>>>> SLAB_TYPESAFE_BY_RCU, old readers may still hold references to an object
> >>>>>> even though it is just now being added, which means the modification of
> >>>>>> ->next is visible to readers.  This patch therefore uses WRITE_ONCE()
> >>>>>> for assignments to ->next.
> >>>>>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> >>>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >>>>> 
> >>>>> Did we ever conclude that the READ_ONCE() counterparts were not needed? ;-)
> >>>> 
> >>>> Read-side is already protected by rcu_dereference_raw() in hlist_nulls_for_each_entry_{rcu, safe}.
> >>> 
> >>> It turns out that different traversal synchronization designs want
> >>> different pointers using WRITE_ONCE().
> >> 
> >> Thank you Alan and Paul,
> >> 
> >> Btw, I don't see any users of hlist_nulls_unhashed_lockless(), maybe it can
> >> be removed?
> > 
> > Either that or the people who removed uses injected bugs...
> 
> It has never been used.
> 
> That said, the data race has been there almost for four years.
> 
> And the network people use sk_unhashed() for both hlist_node and hlist_nulls_node.
> So, I plan to use hlist_unhashed_lockless() in sk_unhashed(), that will be one of my future patches.
> 
> > 
> > But if this one really does go away, do we need ->pprev to be
> > protected by _ONCE()?
> 
> The ->pprev thing is what I’m currently working on. :)

Very good, looking forward to seeing what you come up with!

							Thanx, Paul
diff mbox series

Patch

diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index ba4c00dd8005..89186c499dd4 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -101,7 +101,7 @@  static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
 {
 	struct hlist_nulls_node *first = h->first;
 
-	n->next = first;
+	WRITE_ONCE(n->next, first);
 	WRITE_ONCE(n->pprev, &h->first);
 	rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
 	if (!is_a_nulls(first))
@@ -137,7 +137,7 @@  static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
 		last = i;
 
 	if (last) {
-		n->next = last->next;
+		WRITE_ONCE(n->next, last->next);
 		n->pprev = &last->next;
 		rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
 	} else {