Message ID | 20230717180317.1097590-6-paulmck@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp1274482vqt; Mon, 17 Jul 2023 11:24:41 -0700 (PDT) X-Google-Smtp-Source: APBJJlFJ3gaCfdvGcG3OHZn8AkxKN1Q5pFAuzlpnATNzkEjPrxlUivUQDaY2vJF3QvpLTDUSl6yA X-Received: by 2002:a17:906:73c4:b0:994:536c:ab45 with SMTP id n4-20020a17090673c400b00994536cab45mr7996393ejl.50.1689618281467; Mon, 17 Jul 2023 11:24:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689618281; cv=none; d=google.com; s=arc-20160816; b=dFUCNRZFByHwTzA/qxO2GXfs8FFK3zwL7d77klCLzjMDsZ05re3f9uEbeHg5Y4inDz axJfs2JbgR1kNQdL2bwkmjXYcp4e2F+XCYbtsH7oEfNIHw+XWSqg3Ei5xP1WkoQ0n21e 50MXdrDEYOU3549mcdii9hOW3pOVj/fYRBQmEm6IPL+WGIytwDR6oLjoQuSPccfqoIam fl0Aq4YfMhgHowlgBUcWfDyrTyPe9iEXlfH1maGLF/w9xuj7xVKxaV/OYGcw0KI028Fu nW9gSYZee3oiEtYiqvjNCRUvpnQ3/3hunHblb2r+WvCjvEh1wYiH9Xwg2IMSXRla37Bf d6sg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=mia4pCzhNuiW3y6T/7Js7WYUleKkbgZy2/l1yU1jMC4=; fh=Lb56TjNevSGTBIMzcPu79Y+BXL+OVlD7s9uUE6Q7sFQ=; b=MdndoEghSHGCRZtX8L+NQvqCuku+0VJEszobgqRlzDhCHEN7J594PairumBWLMh7VT mwI6lL5q3yZH1Zh/c4yJGYsJoZkRpiH82ji6VfK230PAB5scCGJ9UTfTZ6asFEyerz59 jzs2Tqsi/8EihLLLDvvHQMt0Y0hfT88idAxYrrnE4k0Y3id1JwG7DPFofpCTBfk9SXgG DHVBJ5s4mP2P6Vu1bO33xEY7I1FduAWVqxZXuEma/5ODhHagNSaVhY9/mgydSGBA0vFV Jsjp5xVul4vh0HsNxwlBvkl3v9VW8KMaWPfmjhwgmlJctg17BCKn96sYsPujwzfP0gJM hz7A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="dwsvUF/M"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e25-20020a170906081900b00997672cb9d7si12863ejd.411.2023.07.17.11.24.16; Mon, 17 Jul 2023 11:24:41 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="dwsvUF/M"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231128AbjGQSDh (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Mon, 17 Jul 2023 14:03:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34556 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229970AbjGQSDV (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Jul 2023 14:03:21 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D49B499; Mon, 17 Jul 2023 11:03:20 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id AA8A9611C5; Mon, 17 Jul 2023 18:03:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E07CAC4339A; Mon, 17 Jul 2023 18:03:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689616998; bh=vEUUOY6M/3b2MdG2PW8TiiGilUJqe947oj/T9vFTNTA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=dwsvUF/M6y1htH9x4F8FD9RJ7dMM3iqjK0L9G/KO/bHFbfAbvKW9LQqgdffTCkrJ8 Q3VAWs2tJvzYG01qCc+I0khfxeLsX5IXuA1qB7E7B6bmN6KzPvg0NDwchVgnJZIkRt l/BgTtbHEtZ8VzJoDXONq4GP1/9sdbeAh16Ap3IUrKuqUoEdH5IIJkkIX1TXbu5hZI ZMocGgOmJXnGa8S6m/1xMih1P4I3vb6g6tBoxrHzFJD1HGwLxm8KgklhyGYt0jh8x3 PoBhOvFNITxVPY0ovU5TqzlWXzTrJ9FCy1Vy3qu9PymxoGgX6Z0t1iYAQmHbcobG8s ZyVrbtuztfxPw== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id 441F6CE092F; Mon, 17 Jul 2023 11:03:18 -0700 (PDT) From: "Paul E. McKenney" <paulmck@kernel.org> To: rcu@vger.kernel.org Cc: linux-kernel@vger.kernel.org, kernel-team@meta.com, rostedt@goodmis.org, Alan Huang <mmpgouride@gmail.com>, "Paul E . McKenney" <paulmck@kernel.org> Subject: [PATCH rcu 6/6] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Date: Mon, 17 Jul 2023 11:03:17 -0700 Message-Id: <20230717180317.1097590-6-paulmck@kernel.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <6127192c-da9b-4599-9738-6e8f92e6c75c@paulmck-laptop> References: <6127192c-da9b-4599-9738-6e8f92e6c75c@paulmck-laptop> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771693178594996270 X-GMAIL-MSGID: 1771693178594996270 |
Series |
Miscellaneous fixes for v6.6
|
|
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
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 {
> 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 { >
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 { > > >
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
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
> 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
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 --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 {