Paul E. McKenney
2013-Oct-09 21:29 UTC
[Bridge] [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
Hello! This series features updates to allow sparse to do a better job of statically analyzing RCU usage: 1. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent comiler mischief. Also require that the source pointer be from the kernel address space. Sometimes it can be from the RCU address space, which necessitates the remaining patches in this series. Which, it must be admitted, apply to a very small fraction of the rcu_assign_pointer() invocations in the kernel. This commit courtesy of Josh Triplett. 2-13. Apply rcu_access_pointer() to avoid a number of false positives. Changes from v1: o Fix grammar nit in commit logs. Thanx, Paul b/drivers/net/bonding/bond_alb.c | 3 ++- b/drivers/net/bonding/bond_main.c | 8 +++++--- b/include/linux/rcupdate.h | 12 +++++++++++- b/kernel/notifier.c | 2 +- b/net/bridge/br_mdb.c | 2 +- b/net/bridge/br_multicast.c | 4 ++-- b/net/decnet/dn_route.c | 5 +++-- b/net/ipv4/ip_sockglue.c | 2 +- b/net/ipv6/ip6_gre.c | 2 +- b/net/ipv6/ip6_tunnel.c | 2 +- b/net/ipv6/sit.c | 2 +- b/net/mac80211/sta_info.c | 4 ++-- b/net/wireless/scan.c | 14 +++++++------- 13 files changed, 38 insertions(+), 24 deletions(-)
Paul E. McKenney
2013-Oct-09 21:29 UTC
[Bridge] [PATCH v2 tip/core/rcu 03/13] bridge: Apply rcu_access_pointer() to avoid sparse false positive
From: "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> The sparse checking for rcu_assign_pointer() was recently upgraded to reject non-__kernel address spaces. This also rejects __rcu, which is almost always the right thing to do. However, the uses in br_multicast_del_pg() and br_multicast_new_port_group() are legitimate: They are assigning a pointer to an element from an RCU-protected list, and all elements of this list are already visible to caller. This commit therefore silences these false positives by laundering the pointers using rcu_access_pointer() as suggested by Josh Triplett. Reported-by: kbuild test robot <fengguang.wu at intel.com> Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> Cc: Stephen Hemminger <stephen at networkplumber.org> Cc: "David S. Miller" <davem at davemloft.net> Cc: bridge at lists.linux-foundation.org Cc: netdev at vger.kernel.org --- net/bridge/br_multicast.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index d1c578630678..314c81cc5855 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -267,7 +267,7 @@ static void br_multicast_del_pg(struct net_bridge *br, if (p != pg) continue; - rcu_assign_pointer(*pp, p->next); + rcu_assign_pointer(*pp, rcu_access_pointer(p->next)); hlist_del_init(&p->mglist); del_timer(&p->timer); call_rcu_bh(&p->rcu, br_multicast_free_pg); @@ -646,7 +646,7 @@ struct net_bridge_port_group *br_multicast_new_port_group( p->addr = *group; p->port = port; p->state = state; - rcu_assign_pointer(p->next, next); + rcu_assign_pointer(p->next, rcu_access_pointer(next)); hlist_add_head(&p->mglist, &port->mglist); setup_timer(&p->timer, br_multicast_port_group_expired, (unsigned long)p); -- 1.8.1.5
Paul E. McKenney
2013-Oct-09 21:29 UTC
[PATCH v2 tip/core/rcu 04/13] wireless: Apply rcu_access_pointer() to avoid sparse false positive
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> The sparse checking for rcu_assign_pointer() was recently upgraded to reject non-__kernel address spaces. This also rejects __rcu, which is almost always the right thing to do. However, the uses in cfg80211_combine_bsses() and cfg80211_bss_update() are legitimate: They are assigning a pointer to an element from an RCU-protected list, and all elements of this list are already visible to caller. This commit therefore silences these false positives by laundering the pointers using rcu_access_pointer() as suggested by Josh Triplett. Reported-by: kbuild test robot <fengguang.wu@intel.com> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: Stephen Hemminger <stephen@networkplumber.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: bridge@lists.linux-foundation.org Cc: netdev@vger.kernel.org --- net/wireless/scan.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/net/wireless/scan.c b/net/wireless/scan.c index eeb71480f1af..edde117c1863 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -671,7 +671,7 @@ static bool cfg80211_combine_bsses(struct cfg80211_registered_device *dev, bss->pub.hidden_beacon_bss = &new->pub; new->refcount += bss->refcount; rcu_assign_pointer(bss->pub.beacon_ies, - new->pub.beacon_ies); + rcu_access_pointer(new->pub.beacon_ies)); } return true; @@ -706,10 +706,10 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, old = rcu_access_pointer(found->pub.proberesp_ies); rcu_assign_pointer(found->pub.proberesp_ies, - tmp->pub.proberesp_ies); + rcu_access_pointer(tmp->pub.proberesp_ies)); /* Override possible earlier Beacon frame IEs */ rcu_assign_pointer(found->pub.ies, - tmp->pub.proberesp_ies); + rcu_access_pointer(tmp->pub.proberesp_ies)); if (old) kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head); @@ -740,12 +740,12 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, old = rcu_access_pointer(found->pub.beacon_ies); rcu_assign_pointer(found->pub.beacon_ies, - tmp->pub.beacon_ies); + rcu_access_pointer(tmp->pub.beacon_ies)); /* Override IEs if they were from a beacon before */ if (old == rcu_access_pointer(found->pub.ies)) rcu_assign_pointer(found->pub.ies, - tmp->pub.beacon_ies); + rcu_access_pointer(tmp->pub.beacon_ies)); /* Assign beacon IEs to all sub entries */ list_for_each_entry(bss, &found->hidden_list, @@ -756,7 +756,7 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, WARN_ON(ies != old); rcu_assign_pointer(bss->pub.beacon_ies, - tmp->pub.beacon_ies); + rcu_access_pointer(tmp->pub.beacon_ies)); } if (old) @@ -804,7 +804,7 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, &hidden->hidden_list); hidden->refcount++; rcu_assign_pointer(new->pub.beacon_ies, - hidden->pub.beacon_ies); + rcu_access_pointer(hidden->pub.beacon_ies)); } } else { /* -- 1.8.1.5
Paul E. McKenney
2013-Oct-09 21:29 UTC
[Bridge] [PATCH v2 tip/core/rcu 11/13] bridge/br_mdb: Apply rcu_access_pointer() to avoid sparse false positive
From: "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> The sparse checking for rcu_assign_pointer() was recently upgraded to reject non-__kernel address spaces. This also rejects __rcu, which is almost always the right thing to do. However, the use in __br_mdb_del() is legitimate: They are assigning a pointer to an element from an RCU-protected list, and all elements of this list are already visible to caller. This commit therefore silences these false positives by laundering the pointers using rcu_access_pointer() as suggested by Josh Triplett. Reported-by: kbuild test robot <fengguang.wu at intel.com> Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> Cc: Stephen Hemminger <stephen at networkplumber.org> Cc: "David S. Miller" <davem at davemloft.net> Cc: bridge at lists.linux-foundation.org Cc: netdev at vger.kernel.org --- net/bridge/br_mdb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c index 85a09bb5ca51..3184c8812b49 100644 --- a/net/bridge/br_mdb.c +++ b/net/bridge/br_mdb.c @@ -447,7 +447,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry) if (p->port->state == BR_STATE_DISABLED) goto unlock; - rcu_assign_pointer(*pp, p->next); + rcu_assign_pointer(*pp, rcu_access_pointer(p->next)); hlist_del_init(&p->mglist); del_timer(&p->timer); call_rcu_bh(&p->rcu, br_multicast_free_pg); -- 1.8.1.5
Paul E. McKenney
2013-Oct-09 21:29 UTC
[Bridge] [PATCH v2 tip/core/rcu 12/13] bonding/bond_main: Apply rcu_access_pointer() to avoid sparse false positive
From: "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> The sparse checking for rcu_assign_pointer() was recently upgraded to reject non-__kernel address spaces. This also rejects __rcu, which is almost always the right thing to do. However, the uses in bond_change_active_slave(), bond_enslave(), and __bond_release_one() are legitimate: They are assigning a pointer to an element from an RCU-protected list (or a NULL pointer), and all elements of this list are already visible to caller. This commit therefore silences these false positives either by laundering the pointers using rcu_access_pointer() as suggested by Josh Triplett, or by using RCU_INIT_POINTER() for NULL pointer assignments. Reported-by: kbuild test robot <fengguang.wu at intel.com> Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com> Cc: Stephen Hemminger <stephen at networkplumber.org> Cc: "David S. Miller" <davem at davemloft.net> Cc: bridge at lists.linux-foundation.org Cc: netdev at vger.kernel.org --- drivers/net/bonding/bond_main.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 72df399c4ab3..2f276b971bc4 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -890,7 +890,8 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active) if (new_active) bond_set_slave_active_flags(new_active); } else { - rcu_assign_pointer(bond->curr_active_slave, new_active); + rcu_assign_pointer(bond->curr_active_slave, + rcu_access_pointer(new_active)); } if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) { @@ -1601,7 +1602,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) * so we can change it without calling change_active_interface() */ if (!bond->curr_active_slave && new_slave->link == BOND_LINK_UP) - rcu_assign_pointer(bond->curr_active_slave, new_slave); + rcu_assign_pointer(bond->curr_active_slave, + rcu_access_pointer(new_slave)); break; } /* switch(bond_mode) */ @@ -1801,7 +1803,7 @@ static int __bond_release_one(struct net_device *bond_dev, } if (all) { - rcu_assign_pointer(bond->curr_active_slave, NULL); + RCU_INIT_POINTER(bond->curr_active_slave, NULL); } else if (oldcurrent == slave) { /* * Note that we hold RTNL over this sequence, so there -- 1.8.1.5
Josh Triplett
2013-Oct-09 22:18 UTC
[Bridge] [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.I would suggest moving patch 1 to the end of the series, to avoid introducing and subsequently fixing warnings. - Josh Triplett
Josh Triplett
2013-Oct-09 22:18 UTC
Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()''s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.I would suggest moving patch 1 to the end of the series, to avoid introducing and subsequently fixing warnings. - Josh Triplett
Josh Triplett
2013-Oct-09 22:23 UTC
Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()''s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.The use of rcu_access_pointer directly in the argument of rcu_assign_pointer does add a new constraint to rcu_assign_pointer, namely that it *must not* evaluate its argument more than once. Currently, it expands its argument three times, but one expansion is only visible to sparse (__CHECKER__), and one lives inside a typeof (where it''ll never be evaluated), so this is safe. However, I''d add a comment to that effect above rcu_assign_pointer, explicitly saying that it must evaluate its argument exactly once; that way, if anyone ever changes it, they''ll know they have to introduce an appropriate local temporary variable inside the macro. - Josh Triplett
Josh Triplett
2013-Oct-09 22:23 UTC
[Bridge] [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.The use of rcu_access_pointer directly in the argument of rcu_assign_pointer does add a new constraint to rcu_assign_pointer, namely that it *must not* evaluate its argument more than once. Currently, it expands its argument three times, but one expansion is only visible to sparse (__CHECKER__), and one lives inside a typeof (where it'll never be evaluated), so this is safe. However, I'd add a comment to that effect above rcu_assign_pointer, explicitly saying that it must evaluate its argument exactly once; that way, if anyone ever changes it, they'll know they have to introduce an appropriate local temporary variable inside the macro. - Josh Triplett
Josh Triplett
2013-Oct-09 22:30 UTC
[Bridge] [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.I posted one minor nit in response to one of these patches, but in any case, for 2-13: Reviewed-by: Josh Triplett <josh at joshtriplett.org> I'm obviously OK with patch 1 as well, but it should move to the end of the series, and you need a new patch 1 that adds a comment constraining rcu_assign_pointer to evaluate its argument exactly once. - Josh Triplett
Josh Triplett
2013-Oct-09 22:30 UTC
Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote:> Hello! > > This series features updates to allow sparse to do a better job of > statically analyzing RCU usage: > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()''s target to prevent > comiler mischief. Also require that the source pointer be from > the kernel address space. Sometimes it can be from the RCU address > space, which necessitates the remaining patches in this series. > Which, it must be admitted, apply to a very small fraction of > the rcu_assign_pointer() invocations in the kernel. This commit > courtesy of Josh Triplett. > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives.I posted one minor nit in response to one of these patches, but in any case, for 2-13: Reviewed-by: Josh Triplett <josh@joshtriplett.org> I''m obviously OK with patch 1 as well, but it should move to the end of the series, and you need a new patch 1 that adds a comment constraining rcu_assign_pointer to evaluate its argument exactly once. - Josh Triplett
Paul E. McKenney
2013-Oct-09 22:46 UTC
[Bridge] [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13
On Wed, Oct 09, 2013 at 03:18:05PM -0700, Josh Triplett wrote:> On Wed, Oct 09, 2013 at 02:29:20PM -0700, Paul E. McKenney wrote: > > Hello! > > > > This series features updates to allow sparse to do a better job of > > statically analyzing RCU usage: > > > > 1. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent > > comiler mischief. Also require that the source pointer be from > > the kernel address space. Sometimes it can be from the RCU address > > space, which necessitates the remaining patches in this series. > > Which, it must be admitted, apply to a very small fraction of > > the rcu_assign_pointer() invocations in the kernel. This commit > > courtesy of Josh Triplett. > > > > 2-13. Apply rcu_access_pointer() to avoid a number of false positives. > > I would suggest moving patch 1 to the end of the series, to avoid > introducing and subsequently fixing warnings.That would help with bisectability, will do! Thanx, Paul