| Message ID | 20230706102849.437687-2-mmpgouride@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5ad58b2f997efcdf6221ddbb85816465b093a213 |
| Headers | show |
| Series | rcu: Fix rculist_nulls and doc | expand |
On Thu, Jul 06, 2023 at 10:28:48AM +0000, Alan Huang wrote: > When the objects managed by rculist_nulls are allocated with > SLAB_TYPESAFE_BY_RCU, readers may still hold references to this > object that is being added, which means the modification of ->next > is visible to readers. So, this patch uses WRITE_ONCE() for assignments > to ->next. > > Signed-off-by: Alan Huang <mmpgouride@gmail.com> Very good, queued for the v6.6 merge window, thank you! Thanx, Paul > --- > 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 { > -- > 2.34.1 >
On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote: > > When the objects managed by rculist_nulls are allocated with > SLAB_TYPESAFE_BY_RCU, readers may still hold references to this > object that is being added, which means the modification of ->next > is visible to readers. So, this patch uses WRITE_ONCE() for assignments > to ->next. > > Signed-off-by: Alan Huang <mmpgouride@gmail.com> > --- > 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 { Don't you need READ_ONCE() for the read-side accesses as well? thanks, - Joel
On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote: > On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote: > > > > When the objects managed by rculist_nulls are allocated with > > SLAB_TYPESAFE_BY_RCU, readers may still hold references to this > > object that is being added, which means the modification of ->next > > is visible to readers. So, this patch uses WRITE_ONCE() for assignments > > to ->next. > > > > Signed-off-by: Alan Huang <mmpgouride@gmail.com> > > --- > > 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 { > > Don't you need READ_ONCE() for the read-side accesses as well? Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe() use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu() and hlist_nulls_next_rcu() look rather strange. I would have expected them to also use rcu_dereference_raw(). Thanx, Paul
> 2023年7月11日 04:01，Joel Fernandes <joel@joelfernandes.org> 写道： > > On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote: >> >> When the objects managed by rculist_nulls are allocated with >> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this >> object that is being added, which means the modification of ->next >> is visible to readers. So, this patch uses WRITE_ONCE() for assignments >> to ->next. >> >> Signed-off-by: Alan Huang <mmpgouride@gmail.com> >> --- >> 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 { > > Don't you need READ_ONCE() for the read-side accesses as well? Like Paul said, read-side uses rcu_dereference_raw() > > thanks, > > - Joel
> 2023年7月11日 04:30，Paul E. McKenney <paulmck@kernel.org> 写道： > > On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote: >> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote: >>> >>> When the objects managed by rculist_nulls are allocated with >>> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this >>> object that is being added, which means the modification of ->next >>> is visible to readers. So, this patch uses WRITE_ONCE() for assignments >>> to ->next. >>> >>> Signed-off-by: Alan Huang <mmpgouride@gmail.com> >>> --- >>> 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 { >> >> Don't you need READ_ONCE() for the read-side accesses as well? > > Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe() > use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu() > and hlist_nulls_next_rcu() look rather strange. I would have expected > them to also use rcu_dereference_raw(). hlist_nulls_first_rcu() and hlist_nulls_next_rcu() were added in commit: 67bdbffd696f(“rculist: avoid __rcu annotations”) According to the commit message, this avoids warnings from missing __rcu annotations. > > Thanx, Paul
On Tue, Jul 11, 2023 at 10:56:02PM +0800, Alan Huang wrote: > > > 2023年7月11日 04:30，Paul E. McKenney <paulmck@kernel.org> 写道： > > > > On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote: > >> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote: > >>> > >>> When the objects managed by rculist_nulls are allocated with > >>> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this > >>> object that is being added, which means the modification of ->next > >>> is visible to readers. So, this patch uses WRITE_ONCE() for assignments > >>> to ->next. > >>> > >>> Signed-off-by: Alan Huang <mmpgouride@gmail.com> > >>> --- > >>> 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 { > >> > >> Don't you need READ_ONCE() for the read-side accesses as well? > > > > Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe() > > use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu() > > and hlist_nulls_next_rcu() look rather strange. I would have expected > > them to also use rcu_dereference_raw(). > > hlist_nulls_first_rcu() and hlist_nulls_next_rcu() were added in commit: > > 67bdbffd696f(“rculist: avoid __rcu annotations”) > > According to the commit message, this avoids warnings from missing __rcu annotations. Indeed it does, apologies for the noise! Thanx, Paul
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 {
When the objects managed by rculist_nulls are allocated with SLAB_TYPESAFE_BY_RCU, readers may still hold references to this object that is being added, which means the modification of ->next is visible to readers. So, this patch uses WRITE_ONCE() for assignments to ->next. Signed-off-by: Alan Huang <mmpgouride@gmail.com> --- include/linux/rculist_nulls.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)