Paul E. McKenney
2013-Oct-11 23:16 UTC
[Bridge] [PATCH v3 tip/core/rcu 0/14] 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. Add a comment indicating that despite appearances, rcu_assign_pointer() really only evaluates its arguments once, as a cpp macro should. 2-13. Apply ACCESS_ONCE() to avoid a number of rcu_assign_pointer() calls that would otherwise suffer sparse false positives given patch #13 below. 14. 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. Changes from v2: o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that the pointers are all --rcu and already visible to readers, as suggested by Eric Dumazet and Josh Triplett. o Place the commit adding the rcu_assign_pointer()'s ACCESS_ONCE() at the end to allow better bisectability, as suggested by Josh Triplett. o Add a comment to rcu_assign_pointer() noting that it only evaluates its arguments once, as suggested by Josh Triplett. 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 | 20 +++++++++++++++++++- b/kernel/notifier.c | 3 ++- b/net/bridge/br_mdb.c | 2 +- b/net/bridge/br_multicast.c | 4 ++-- b/net/decnet/dn_route.c | 8 +++++--- b/net/ipv4/ip_sockglue.c | 3 ++- b/net/ipv6/ip6_gre.c | 3 ++- b/net/ipv6/ip6_tunnel.c | 3 ++- b/net/ipv6/sit.c | 3 ++- b/net/mac80211/sta_info.c | 7 ++++--- b/net/wireless/scan.c | 32 ++++++++++++++++++-------------- 13 files changed, 66 insertions(+), 33 deletions(-)
Paul E. McKenney
2013-Oct-11 23:17 UTC
[Bridge] [PATCH v3 tip/core/rcu 03/14] bridge: Apply ACCESS_ONCE() 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 ACCESS_ONCE() as suggested by Eric Dumazet and 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..bcc4bbc16498 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); + ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */ 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); + ACCESS_ONCE(p->next) = next; /* OK: Both --rcu and visible. */ 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-11 23:17 UTC
[Bridge] [PATCH v3 tip/core/rcu 04/14] wireless: Apply ACCESS_ONCE() 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 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 ACCESS_ONCE() as suggested by Eric Dumazet and 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/wireless/scan.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/net/wireless/scan.c b/net/wireless/scan.c index eeb71480f1af..ac3a47abf195 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -670,8 +670,8 @@ static bool cfg80211_combine_bsses(struct cfg80211_registered_device *dev, list_add(&bss->hidden_list, &new->hidden_list); bss->pub.hidden_beacon_bss = &new->pub; new->refcount += bss->refcount; - rcu_assign_pointer(bss->pub.beacon_ies, - new->pub.beacon_ies); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(bss->pub.beacon_ies) = new->pub.beacon_ies; } return true; @@ -705,11 +705,12 @@ 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); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(found->pub.proberesp_ies) + tmp->pub.proberesp_ies; /* Override possible earlier Beacon frame IEs */ - rcu_assign_pointer(found->pub.ies, - tmp->pub.proberesp_ies); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(found->pub.ies) = tmp->pub.proberesp_ies; if (old) kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head); @@ -739,13 +740,14 @@ 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); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(found->pub.beacon_ies) = 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); + /* Both --rcu & visible, ACCESS_ONCE() is OK. */ + ACCESS_ONCE(found->pub.ies) + tmp->pub.beacon_ies; /* Assign beacon IEs to all sub entries */ list_for_each_entry(bss, &found->hidden_list, @@ -755,8 +757,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, ies = rcu_access_pointer(bss->pub.beacon_ies); WARN_ON(ies != old); - rcu_assign_pointer(bss->pub.beacon_ies, - tmp->pub.beacon_ies); + /* Both --rcu & visible, ACCESS_ONCE() is OK. */ + ACCESS_ONCE(bss->pub.beacon_ies) + tmp->pub.beacon_ies; } if (old) @@ -803,8 +806,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev, list_add(&new->hidden_list, &hidden->hidden_list); hidden->refcount++; - rcu_assign_pointer(new->pub.beacon_ies, - hidden->pub.beacon_ies); + /* Both --rcu & visible, ACCESS_ONCE() is OK. */ + ACCESS_ONCE(new->pub.beacon_ies) + hidden->pub.beacon_ies; } } else { /* -- 1.8.1.5
Paul E. McKenney
2013-Oct-11 23:17 UTC
[Bridge] [PATCH v3 tip/core/rcu 11/14] bridge/br_mdb: Apply ACCESS_ONCE() 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 ACCESS_ONCE() as suggested by Eric Dumazet and 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..de7197ba8695 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); + ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */ 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-11 23:17 UTC
[Bridge] [PATCH v3 tip/core/rcu 12/14] bonding/bond_main: Apply ACCESS_ONCE() 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 ACCESS_ONCE() as suggested by Eric Dumazet and 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..e4270ae1c0a8 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); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(bond->curr_active_slave) = 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); + /* Both --rcu and visible, so ACCESS_ONCE() is OK. */ + ACCESS_ONCE(bond->curr_active_slave) = 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-12 06:53 UTC
Re: [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13
On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:> Changes from v2: > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that > the pointers are all --rcu and already visible to readers, > as suggested by Eric Dumazet and Josh Triplett.Hang on a moment. Do *none* of these cases need write memory barriers? - Josh Triplet
Josh Triplett
2013-Oct-12 06:53 UTC
[Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13
On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:> Changes from v2: > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that > the pointers are all --rcu and already visible to readers, > as suggested by Eric Dumazet and Josh Triplett.Hang on a moment. Do *none* of these cases need write memory barriers? - Josh Triplet
Paul E. McKenney
2013-Oct-12 17:13 UTC
[Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13
On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:> On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote: > > Changes from v2: > > > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that > > the pointers are all --rcu and already visible to readers, > > as suggested by Eric Dumazet and Josh Triplett. > > Hang on a moment. Do *none* of these cases need write memory barriers?Sigh. Some afternoons it doesn't pay to touch the keyboard. Thank you for catching this. I will fix, but at this point, I am thinking in terms of 3.14 rather than 3.13 for this series. Thanx, Paul
Hannes Frederic Sowa
2013-Oct-12 17:39 UTC
Re: [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13
On Sat, Oct 12, 2013 at 10:13:45AM -0700, Paul E. McKenney wrote:> On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote: > > On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote: > > > Changes from v2: > > > > > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that > > > the pointers are all --rcu and already visible to readers, > > > as suggested by Eric Dumazet and Josh Triplett. > > > > Hang on a moment. Do *none* of these cases need write memory barriers? > > Sigh. Some afternoons it doesn''t pay to touch the keyboard. > > Thank you for catching this. I will fix, but at this point, I am thinking > in terms of 3.14 rather than 3.13 for this series.Some of them looked safe. You could also replace --rcu with __rcu in the comments while at it. Thanks, Hannes