[PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[Paul E. McKenney]

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(-)

[PATCH v2 tip/core/rcu 03/13] bridge: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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
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@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/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

[PATCH v2 tip/core/rcu 04/13] wireless: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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

[PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
dn_insert_route() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Stephen Hemminger <shemminger@vyatta.com>
Cc: linux-decnet-user@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
 net/decnet/dn_route.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index fe32388ea24f..3b1357bcfc92 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -345,7 +345,7 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
 			/* Put it first */
 			*rthp = rth->dst.dn_next;
 			rcu_assign_pointer(rth->dst.dn_next,
-					   dn_rt_hash_table[hash].chain);
+					   rcu_access_pointer(dn_rt_hash_table[hash].chain));
 			rcu_assign_pointer(dn_rt_hash_table[hash].chain, rth);
 
 			dst_use(&rth->dst, now);
@@ -358,7 +358,8 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
 		rthp = &rth->dst.dn_next;
 	}
 
-	rcu_assign_pointer(rt->dst.dn_next, dn_rt_hash_table[hash].chain);
+	rcu_assign_pointer(rt->dst.dn_next,
+			   rcu_access_pointer(dn_rt_hash_table[hash].chain));
 	rcu_assign_pointer(dn_rt_hash_table[hash].chain, rt);
 
 	dst_use(&rt->dst, now);
-- 
1.8.1.5

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Josh Triplett]

On Wed, Oct 09, 2013 at 02:29:38PM -0700, Paul E. McKenney wrote:
> 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 use in
> dn_insert_route() is legitimate: It is assigning a pointer to an element

Nit: "uses ... are", not "use ... is". :)

> from an RCU-protected list, and all elements of this list are already
> visible to caller.
> 
> This commit therefore silences this false positive by laundering the
> pointer 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: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Gao feng <gaofeng@cn.fujitsu.com>
> Cc: Stephen Hemminger <shemminger@vyatta.com>
> Cc: linux-decnet-user@lists.sourceforge.net
> Cc: netdev@vger.kernel.org

With or without the above typo fix:

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  net/decnet/dn_route.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
> index fe32388ea24f..3b1357bcfc92 100644
> --- a/net/decnet/dn_route.c
> +++ b/net/decnet/dn_route.c
> @@ -345,7 +345,7 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
>  			/* Put it first */
>  			*rthp = rth->dst.dn_next;
>  			rcu_assign_pointer(rth->dst.dn_next,
> -					   dn_rt_hash_table[hash].chain);
> +					   rcu_access_pointer(dn_rt_hash_table[hash].chain));
>  			rcu_assign_pointer(dn_rt_hash_table[hash].chain, rth);
>  
>  			dst_use(&rth->dst, now);
> @@ -358,7 +358,8 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
>  		rthp = &rth->dst.dn_next;
>  	}
>  
> -	rcu_assign_pointer(rt->dst.dn_next, dn_rt_hash_table[hash].chain);
> +	rcu_assign_pointer(rt->dst.dn_next,
> +			   rcu_access_pointer(dn_rt_hash_table[hash].chain));
>  	rcu_assign_pointer(dn_rt_hash_table[hash].chain, rt);
>  
>  	dst_use(&rt->dst, now);
> -- 
> 1.8.1.5
> 

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 03:28:42PM -0700, Josh Triplett wrote:
> On Wed, Oct 09, 2013 at 02:29:38PM -0700, Paul E. McKenney wrote:
> > 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 use in
> > dn_insert_route() is legitimate: It is assigning a pointer to an element
> 
> Nit: "uses ... are", not "use ... is". :)

Don't I already have "use ... is"?

							Thanx, Paul

> > from an RCU-protected list, and all elements of this list are already
> > visible to caller.
> > 
> > This commit therefore silences this false positive by laundering the
> > pointer 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: "David S. Miller" <davem@davemloft.net>
> > Cc: Thomas Graf <tgraf@suug.ch>
> > Cc: Gao feng <gaofeng@cn.fujitsu.com>
> > Cc: Stephen Hemminger <shemminger@vyatta.com>
> > Cc: linux-decnet-user@lists.sourceforge.net
> > Cc: netdev@vger.kernel.org
> 
> With or without the above typo fix:
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 
> > ---
> >  net/decnet/dn_route.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
> > index fe32388ea24f..3b1357bcfc92 100644
> > --- a/net/decnet/dn_route.c
> > +++ b/net/decnet/dn_route.c
> > @@ -345,7 +345,7 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
> >  			/* Put it first */
> >  			*rthp = rth->dst.dn_next;
> >  			rcu_assign_pointer(rth->dst.dn_next,
> > -					   dn_rt_hash_table[hash].chain);
> > +					   rcu_access_pointer(dn_rt_hash_table[hash].chain));
> >  			rcu_assign_pointer(dn_rt_hash_table[hash].chain, rth);
> >  
> >  			dst_use(&rth->dst, now);
> > @@ -358,7 +358,8 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
> >  		rthp = &rth->dst.dn_next;
> >  	}
> >  
> > -	rcu_assign_pointer(rt->dst.dn_next, dn_rt_hash_table[hash].chain);
> > +	rcu_assign_pointer(rt->dst.dn_next,
> > +			   rcu_access_pointer(dn_rt_hash_table[hash].chain));
> >  	rcu_assign_pointer(dn_rt_hash_table[hash].chain, rt);
> >  
> >  	dst_use(&rt->dst, now);
> > -- 
> > 1.8.1.5
> > 
> 

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Josh Triplett]

On Wed, Oct 09, 2013 at 03:46:04PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 09, 2013 at 03:28:42PM -0700, Josh Triplett wrote:
> > On Wed, Oct 09, 2013 at 02:29:38PM -0700, Paul E. McKenney wrote:
> > > 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 use in
> > > dn_insert_route() is legitimate: It is assigning a pointer to an element
> > 
> > Nit: "uses ... are", not "use ... is". :)
> 
> Don't I already have "use ... is"?

I was suggesting that it needed to change from "use ... is" to "uses ...
are", not the other way around.

- Josh Triplett

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 03:57:16PM -0700, Josh Triplett wrote:
> On Wed, Oct 09, 2013 at 03:46:04PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 09, 2013 at 03:28:42PM -0700, Josh Triplett wrote:
> > > On Wed, Oct 09, 2013 at 02:29:38PM -0700, Paul E. McKenney wrote:
> > > > 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 use in
> > > > dn_insert_route() is legitimate: It is assigning a pointer to an element
> > > 
> > > Nit: "uses ... are", not "use ... is". :)
> > 
> > Don't I already have "use ... is"?
> 
> I was suggesting that it needed to change from "use ... is" to "uses ...
> are", not the other way around.

I guess there really are two uses rather than just one.  ;-)

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Dhaval Giani]

On Wed, Oct 9, 2013 at 5:29 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> 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 use in
> dn_insert_route() is legitimate: It is 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 this false positive by laundering the
> pointer using rcu_access_pointer() as suggested by Josh Triplett.
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>


I did not realize that we were allowed to rename people :-)

Thanks!
Dhaval

Re: [PATCH v2 tip/core/rcu 05/13] decnet: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 06:58:47PM -0400, Dhaval Giani wrote:
> On Wed, Oct 9, 2013 at 5:29 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > 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 use in
> > dn_insert_route() is legitimate: It is 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 this false positive by laundering the
> > pointer using rcu_access_pointer() as suggested by Josh Triplett.
> >
> > Reported-by: kbuild test robot <fengguang.wu@intel.com>
> 
> I did not realize that we were allowed to rename people :-)

Copied and pasted directly from the email I received.  Perhaps strange,
but true!  ;-)

							Thanx, Paul

[PATCH v2 tip/core/rcu 06/13] ipv4/ip_socketglue: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
ip_ra_control() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv4/ip_sockglue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index d9c4f113d709..d328f158f0e5 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -269,7 +269,7 @@ int ip_ra_control(struct sock *sk, unsigned char on,
 			}
 			/* dont let ip_call_ra_chain() use sk again */
 			ra->sk = NULL;
-			rcu_assign_pointer(*rap, ra->next);
+			rcu_assign_pointer(*rap, rcu_access_pointer(ra->next));
 			spin_unlock_bh(&ip_ra_lock);
 
 			if (ra->destructor)
-- 
1.8.1.5

[PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
ip6_tnl_unlink() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/ip6_tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 61355f7f4da5..ecc0166e1a9c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			rcu_assign_pointer(*tp, rcu_access_pointer(t->next));
 			break;
 		}
 	}
-- 
1.8.1.5

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Eric Dumazet]

On Wed, 2013-10-09 at 14:29 -0700, Paul E. McKenney wrote:
> 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 use in
> ip6_tnl_unlink() is legitimate: It is 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 this false positive by laundering the
> pointer 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: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: netdev@vger.kernel.org
> ---
>  net/ipv6/ip6_tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 61355f7f4da5..ecc0166e1a9c 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
>  	     (iter = rtnl_dereference(*tp)) != NULL;
>  	     tp = &iter->next) {
>  		if (t == iter) {
> -			rcu_assign_pointer(*tp, t->next);
> +			rcu_assign_pointer(*tp, rcu_access_pointer(t->next));
>  			break;
>  		}
>  	}

Then it seems a mere "*tp = t->next;" would be enough  ?

We do not really need a barrier.

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 02:42:29PM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-09 at 14:29 -0700, Paul E. McKenney wrote:
> > 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 use in
> > ip6_tnl_unlink() is legitimate: It is 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 this false positive by laundering the
> > pointer 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: "David S. Miller" <davem@davemloft.net>
> > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> > Cc: Patrick McHardy <kaber@trash.net>
> > Cc: netdev@vger.kernel.org
> > ---
> >  net/ipv6/ip6_tunnel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> > index 61355f7f4da5..ecc0166e1a9c 100644
> > --- a/net/ipv6/ip6_tunnel.c
> > +++ b/net/ipv6/ip6_tunnel.c
> > @@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
> >  	     (iter = rtnl_dereference(*tp)) != NULL;
> >  	     tp = &iter->next) {
> >  		if (t == iter) {
> > -			rcu_assign_pointer(*tp, t->next);
> > +			rcu_assign_pointer(*tp, rcu_access_pointer(t->next));
> >  			break;
> >  		}
> >  	}
> 
> Then it seems a mere "*tp = t->next;" would be enough  ?
> 
> We do not really need a barrier.

Hmmm...  I could use RCU_INIT_POINTER().  Something like the following?

	RCU_INIT_POINTER(ACCESS_ONCE(*tp), t->next);

The ACCESS_ONCE() to prevent the compiler from doing anything stupid.
Presumably the value of t->next cannot change, so a normal load suffices.

Or did you have something else in mind?

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Eric Dumazet]

On Wed, 2013-10-09 at 14:57 -0700, Paul E. McKenney wrote:

> Hmmm...  I could use RCU_INIT_POINTER().  Something like the following?
> 
> 	RCU_INIT_POINTER(ACCESS_ONCE(*tp), t->next);
> 
> The ACCESS_ONCE() to prevent the compiler from doing anything stupid.
> Presumably the value of t->next cannot change, so a normal load suffices.
> 
> Or did you have something else in mind?

Well, *tp and t->next are both of the same type, with __rcu attribute.

struct ip6_tnl __rcu **tp;

So I meant :

ACCESS_ONCE(*tp) = t->next;

If really we can have a really stupid compiler.

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 03:10:24PM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-09 at 14:57 -0700, Paul E. McKenney wrote:
> 
> > Hmmm...  I could use RCU_INIT_POINTER().  Something like the following?
> > 
> > 	RCU_INIT_POINTER(ACCESS_ONCE(*tp), t->next);
> > 
> > The ACCESS_ONCE() to prevent the compiler from doing anything stupid.
> > Presumably the value of t->next cannot change, so a normal load suffices.
> > 
> > Or did you have something else in mind?
> 
> Well, *tp and t->next are both of the same type, with __rcu attribute.
> 
> struct ip6_tnl __rcu **tp;
> 
> So I meant :
> 
> ACCESS_ONCE(*tp) = t->next;
> 
> If really we can have a really stupid compiler.

That would work, though it would probably give sparse complaints.

Of course, it is not the stupid compilers that worry me, but rather the
smart ones...

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Eric Dumazet]

On Wed, 2013-10-09 at 15:36 -0700, Paul E. McKenney wrote:

> That would work, though it would probably give sparse complaints.

No sparse error here, as I said types are correct and SPARSE_RCU ready :

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 583b77e..28f8495 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
             (iter = rtnl_dereference(*tp)) != NULL;
             tp = &iter->next) {
                if (t == iter) {
-                       rcu_assign_pointer(*tp, t->next);
+                       ACCESS_ONCE(*tp) = t->next;
                        break;
                }
        }

root@edumazet-glaptop:/usr/src/net-next# grep CONFIG_SPARSE_RCU_POINTER .config
CONFIG_SPARSE_RCU_POINTER=y

root@edumazet-glaptop:/usr/src/net-next# make C=2 net/ipv6/ip6_tunnel.o
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `relocs'.
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHECK   scripts/mod/empty.c
  CHECK   net/ipv6/ip6_tunnel.c

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Josh Triplett]

On Wed, Oct 09, 2013 at 03:51:17PM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-09 at 15:36 -0700, Paul E. McKenney wrote:
> 
> > That would work, though it would probably give sparse complaints.
> 
> No sparse error here, as I said types are correct and SPARSE_RCU ready :
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 583b77e..28f8495 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
>              (iter = rtnl_dereference(*tp)) != NULL;
>              tp = &iter->next) {
>                 if (t == iter) {
> -                       rcu_assign_pointer(*tp, t->next);
> +                       ACCESS_ONCE(*tp) = t->next;
>                         break;
>                 }
>         }

I'd be really hesitant to introduce that type of direct assignment to an
__rcu pointer without wrapping it in some appropriately named macro, or
at the very least adding a comment.

- Josh Triplett

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Eric Dumazet]

On Wed, 2013-10-09 at 15:56 -0700, Josh Triplett wrote:

> I'd be really hesitant to introduce that type of direct assignment to an
> __rcu pointer without wrapping it in some appropriately named macro, or
> at the very least adding a comment.

Well, there is no special magic here, in this specific case :

- deleting an item in an rcu list

Check list_del_rcu(), and you'll notice there is no _barrier_

Adding correct barriers is good, but please do not add them when not
needed.

It makes code hard to understand.


ACCESS(*ptr) = value;

is clear and autodocumented, because it highlights the potential
problem, that is *ptr can be read without any barrier from another cpu.
So we ask the compiler to not write temporary garbage in it.

rcu_assign_pointer(*ptr, rcu_access_pointer(value)) 

is very confusing, because it hides the _real_ problem and add defensive
programming tricks.

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Josh Triplett]

On Wed, Oct 09, 2013 at 04:17:55PM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-09 at 15:56 -0700, Josh Triplett wrote:
> 
> > I'd be really hesitant to introduce that type of direct assignment to an
> > __rcu pointer without wrapping it in some appropriately named macro, or
> > at the very least adding a comment.
> 
> Well, there is no special magic here, in this specific case :
> 
> - deleting an item in an rcu list
> 
> Check list_del_rcu(), and you'll notice there is no _barrier_
> 
> Adding correct barriers is good, but please do not add them when not
> needed.
> 
> It makes code hard to understand.

I'm not arguing for the inclusion of an unnecessary barrier.  I'm
arguing for something more self-documenting than:

> ACCESS(*ptr) = value;

that.  Constructs like list_del_rcu are much clearer, and not
open-coded.  Open-coding synchronization code is almost always a Bad
Idea.

- Josh Triplett

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Eric Dumazet]

On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:

> that.  Constructs like list_del_rcu are much clearer, and not
> open-coded.  Open-coding synchronization code is almost always a Bad
> Idea.

OK, so you think there is synchronization code.

I will shut up then, no need to waste time.

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> 
> > that.  Constructs like list_del_rcu are much clearer, and not
> > open-coded.  Open-coding synchronization code is almost always a Bad
> > Idea.
> 
> OK, so you think there is synchronization code.
> 
> I will shut up then, no need to waste time.

As you said earlier, we should at least get rid of the memory barrier
as long as we are changing the code.

Josh, what would you suggest as the best way to avoid the memory barrier,
keep sparse happy, and not be too ugly?

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Hannes Frederic Sowa]

On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > 
> > > that.  Constructs like list_del_rcu are much clearer, and not
> > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > Idea.
> > 
> > OK, so you think there is synchronization code.
> > 
> > I will shut up then, no need to waste time.
> 
> As you said earlier, we should at least get rid of the memory barrier
> as long as we are changing the code.

Interesting thread!

Sorry to chime in and asking a question:

Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one?
In other words I wonder why rcu_assign_pointer is not a static inline function
to use the sequence point in argument evaluation (if I remember correctly this
also holds for inline functions) to not allow something like this:

E.g. we want to publish which lock to take first to prevent an ABBA problem
(extreme example):

rcu_assign_pointer(lockptr, min(lptr1, lptr2));

Couldn't a compiler spill the lockptr memory location as a temporary buffer
if the compiler is under register pressure? (yes, this seems unlikely if we
flushed out most registers to memory because of the barrier, but still... ;) )

This seems to be also the case if we publish a multi-dereferencing pointers
e.g. ptr->ptr->ptr.

Thanks,

  Hannes

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Thu, Oct 10, 2013 at 04:04:22AM +0200, Hannes Frederic Sowa wrote:
> On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > > 
> > > > that.  Constructs like list_del_rcu are much clearer, and not
> > > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > > Idea.
> > > 
> > > OK, so you think there is synchronization code.
> > > 
> > > I will shut up then, no need to waste time.
> > 
> > As you said earlier, we should at least get rid of the memory barrier
> > as long as we are changing the code.
> 
> Interesting thread!
> 
> Sorry to chime in and asking a question:
> 
> Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one?
> In other words I wonder why rcu_assign_pointer is not a static inline function
> to use the sequence point in argument evaluation (if I remember correctly this
> also holds for inline functions) to not allow something like this:
> 
> E.g. we want to publish which lock to take first to prevent an ABBA problem
> (extreme example):
> 
> rcu_assign_pointer(lockptr, min(lptr1, lptr2));
> 
> Couldn't a compiler spill the lockptr memory location as a temporary buffer
> if the compiler is under register pressure? (yes, this seems unlikely if we
> flushed out most registers to memory because of the barrier, but still... ;) )
> 
> This seems to be also the case if we publish a multi-dereferencing pointers
> e.g. ptr->ptr->ptr.

IIRC, sequence points only confine volatile accesses.  For non-volatile
accesses, the so-called "as-if rule" allows compiler writers to do some
surprisingly global reordering.

The reason that rcu_assign_pointer() isn't an inline function is because
it needs to be type-generic, in other words, it needs to be OK to use
it on any type of pointers as long as the C types of the two pointers
match (the sparse types can vary a bit).

One of the reasons for wanting a volatile cast in rcu_assign_pointer() is
to prevent compiler mischief such as you described in your last two
paragraphs.  That said, it would take a very brave compiler to pull
a pointer-referenced memory location into a register and keep it there.
Unfortunately, increasing compiler bravery seems to be a solid long-term
trend.

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Hannes Frederic Sowa]

On Thu, Oct 10, 2013 at 12:05:32PM -0700, Paul E. McKenney wrote:
> On Thu, Oct 10, 2013 at 04:04:22AM +0200, Hannes Frederic Sowa wrote:
> > On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > > > 
> > > > > that.  Constructs like list_del_rcu are much clearer, and not
> > > > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > > > Idea.
> > > > 
> > > > OK, so you think there is synchronization code.
> > > > 
> > > > I will shut up then, no need to waste time.
> > > 
> > > As you said earlier, we should at least get rid of the memory barrier
> > > as long as we are changing the code.
> > 
> > Interesting thread!
> > 
> > Sorry to chime in and asking a question:
> > 
> > Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one?
> > In other words I wonder why rcu_assign_pointer is not a static inline function
> > to use the sequence point in argument evaluation (if I remember correctly this
> > also holds for inline functions) to not allow something like this:
> > 
> > E.g. we want to publish which lock to take first to prevent an ABBA problem
> > (extreme example):
> > 
> > rcu_assign_pointer(lockptr, min(lptr1, lptr2));
> > 
> > Couldn't a compiler spill the lockptr memory location as a temporary buffer
> > if the compiler is under register pressure? (yes, this seems unlikely if we
> > flushed out most registers to memory because of the barrier, but still... ;) )
> > 
> > This seems to be also the case if we publish a multi-dereferencing pointers
> > e.g. ptr->ptr->ptr.
> 
> IIRC, sequence points only confine volatile accesses.  For non-volatile
> accesses, the so-called "as-if rule" allows compiler writers to do some
> surprisingly global reordering.
> 
> The reason that rcu_assign_pointer() isn't an inline function is because
> it needs to be type-generic, in other words, it needs to be OK to use
> it on any type of pointers as long as the C types of the two pointers
> match (the sparse types can vary a bit).
> 
> One of the reasons for wanting a volatile cast in rcu_assign_pointer() is
> to prevent compiler mischief such as you described in your last two
> paragraphs.  That said, it would take a very brave compiler to pull
> a pointer-referenced memory location into a register and keep it there.
> Unfortunately, increasing compiler bravery seems to be a solid long-term
> trend.

I saw your patch regarding making rcu_assign_pointer volatile and wonder if we
can still make it a bit more safe to use if we force the evaluation of the
to-be-assigned pointer before the write barrier. This is what I have in mind:

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f1f1bc3..79eccc3 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -550,8 +550,9 @@ static inline void rcu_preempt_sleep_check(void)
 	})
 #define __rcu_assign_pointer(p, v, space) \
 	do { \
+		typeof(v) ___v = (v); \
 		smp_wmb(); \
-		(p) = (typeof(*v) __force space *)(v); \
+		(p) = (typeof(*___v) __force space *)(___v); \
 	} while (0)
 
 
I don't think ___v must be volatile for this case because the memory barrier
will force the evaluation of v first.

This would guard against cases where rcu_assign_pointer is used like:

rcu_assign_pointer(ptr, compute_ptr_with_side_effects());

Greetings,

  Hannes

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Sat, Oct 12, 2013 at 04:25:08AM +0200, Hannes Frederic Sowa wrote:
> On Thu, Oct 10, 2013 at 12:05:32PM -0700, Paul E. McKenney wrote:
> > On Thu, Oct 10, 2013 at 04:04:22AM +0200, Hannes Frederic Sowa wrote:
> > > On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> > > > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > > > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > > > > 
> > > > > > that.  Constructs like list_del_rcu are much clearer, and not
> > > > > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > > > > Idea.
> > > > > 
> > > > > OK, so you think there is synchronization code.
> > > > > 
> > > > > I will shut up then, no need to waste time.
> > > > 
> > > > As you said earlier, we should at least get rid of the memory barrier
> > > > as long as we are changing the code.
> > > 
> > > Interesting thread!
> > > 
> > > Sorry to chime in and asking a question:
> > > 
> > > Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one?
> > > In other words I wonder why rcu_assign_pointer is not a static inline function
> > > to use the sequence point in argument evaluation (if I remember correctly this
> > > also holds for inline functions) to not allow something like this:
> > > 
> > > E.g. we want to publish which lock to take first to prevent an ABBA problem
> > > (extreme example):
> > > 
> > > rcu_assign_pointer(lockptr, min(lptr1, lptr2));
> > > 
> > > Couldn't a compiler spill the lockptr memory location as a temporary buffer
> > > if the compiler is under register pressure? (yes, this seems unlikely if we
> > > flushed out most registers to memory because of the barrier, but still... ;) )
> > > 
> > > This seems to be also the case if we publish a multi-dereferencing pointers
> > > e.g. ptr->ptr->ptr.
> > 
> > IIRC, sequence points only confine volatile accesses.  For non-volatile
> > accesses, the so-called "as-if rule" allows compiler writers to do some
> > surprisingly global reordering.
> > 
> > The reason that rcu_assign_pointer() isn't an inline function is because
> > it needs to be type-generic, in other words, it needs to be OK to use
> > it on any type of pointers as long as the C types of the two pointers
> > match (the sparse types can vary a bit).
> > 
> > One of the reasons for wanting a volatile cast in rcu_assign_pointer() is
> > to prevent compiler mischief such as you described in your last two
> > paragraphs.  That said, it would take a very brave compiler to pull
> > a pointer-referenced memory location into a register and keep it there.
> > Unfortunately, increasing compiler bravery seems to be a solid long-term
> > trend.
> 
> I saw your patch regarding making rcu_assign_pointer volatile and wonder if we
> can still make it a bit more safe to use if we force the evaluation of the
> to-be-assigned pointer before the write barrier. This is what I have in mind:
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index f1f1bc3..79eccc3 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -550,8 +550,9 @@ static inline void rcu_preempt_sleep_check(void)
>  	})
>  #define __rcu_assign_pointer(p, v, space) \
>  	do { \
> +		typeof(v) ___v = (v); \
>  		smp_wmb(); \
> -		(p) = (typeof(*v) __force space *)(v); \
> +		(p) = (typeof(*___v) __force space *)(___v); \
>  	} while (0)
> 
> 
> I don't think ___v must be volatile for this case because the memory barrier
> will force the evaluation of v first.
> 
> This would guard against cases where rcu_assign_pointer is used like:
> 
> rcu_assign_pointer(ptr, compute_ptr_with_side_effects());

I am sorry, but I am not seeing how this would be particularly useful.

The point of rcu_assign_pointer() is to order the initialization of
a data structure against publishing a pointer to that data structure.
An example may be found in cgroup_create():

	name = cgroup_alloc_name(dentry);
	if (!name)
		goto err_free_cgrp;
	rcu_assign_pointer(cgrp->name, name);

Here, cgroup_alloc_name() allocates memory for the name and fills in
the name:

static struct cgroup_name *cgroup_alloc_name(struct dentry *dentry)
{
	struct cgroup_name *name;

	name = kmalloc(sizeof(*name) + dentry->d_name.len + 1, GFP_KERNEL);
	if (!name)
		return NULL;
	strcpy(name->name, dentry->d_name.name);
	return name;
}

So the point of the smp_wmb() in __rcu_assign_pointer() is to order the
strcpy() in cgroup_alloc_name() to happen before the assignment of the
name pointer to cgrp->name.

To make this example fit your pattern, we could change the code in
cgroup_create() to look as follows (and to be buggy):

	/* BAD CODE!  Do not do this! */
	rcu_assign_pointer(cgrp->name, cgroup_alloc_name(dentry));
	if (!cgrp->name)
		goto err_free_cgrp;

The reason that this is bad practice is that it is hiding the fact that
the allocation and initialization in cgroup_alloc_name() needs to be
ordered before the assignment to cgrp->name.

Make sense?

							Thanx, Paul

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Hannes Frederic Sowa]

On Sat, Oct 12, 2013 at 12:53:36AM -0700, Paul E. McKenney wrote:
> On Sat, Oct 12, 2013 at 04:25:08AM +0200, Hannes Frederic Sowa wrote:
> > I saw your patch regarding making rcu_assign_pointer volatile and wonder if we
> > can still make it a bit more safe to use if we force the evaluation of the
> > to-be-assigned pointer before the write barrier. This is what I have in mind:
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index f1f1bc3..79eccc3 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -550,8 +550,9 @@ static inline void rcu_preempt_sleep_check(void)
> >  	})
> >  #define __rcu_assign_pointer(p, v, space) \
> >  	do { \
> > +		typeof(v) ___v = (v); \
> >  		smp_wmb(); \
> > -		(p) = (typeof(*v) __force space *)(v); \
> > +		(p) = (typeof(*___v) __force space *)(___v); \
> >  	} while (0)
> > 
> > 
> > I don't think ___v must be volatile for this case because the memory barrier
> > will force the evaluation of v first.
> > 
> > This would guard against cases where rcu_assign_pointer is used like:
> > 
> > rcu_assign_pointer(ptr, compute_ptr_with_side_effects());
> 
> I am sorry, but I am not seeing how this would be particularly useful.

Oh, I do not think it is useful. It should help enforcing the right memory
orderings if someone uses rcu_assign_pointer in such a nasty way.

It was meant by me as a "defensive rearragement" to guard rcu_assign_pointer
to accidentaly slip memory mutations across the barrier without hurting
compiler optimizations too bad (or at all).

> The point of rcu_assign_pointer() is to order the initialization of
> a data structure against publishing a pointer to that data structure.
> An example may be found in cgroup_create():
> 
> 	name = cgroup_alloc_name(dentry);
> 	if (!name)
> 		goto err_free_cgrp;
> 	rcu_assign_pointer(cgrp->name, name);
> 
> Here, cgroup_alloc_name() allocates memory for the name and fills in
> the name:
> 
> static struct cgroup_name *cgroup_alloc_name(struct dentry *dentry)
> {
> 	struct cgroup_name *name;
> 
> 	name = kmalloc(sizeof(*name) + dentry->d_name.len + 1, GFP_KERNEL);
> 	if (!name)
> 		return NULL;
> 	strcpy(name->name, dentry->d_name.name);
> 	return name;
> }
> 
> So the point of the smp_wmb() in __rcu_assign_pointer() is to order the
> strcpy() in cgroup_alloc_name() to happen before the assignment of the
> name pointer to cgrp->name.
> 
> To make this example fit your pattern, we could change the code in
> cgroup_create() to look as follows (and to be buggy):
> 
> 	/* BAD CODE!  Do not do this! */
> 	rcu_assign_pointer(cgrp->name, cgroup_alloc_name(dentry));
> 	if (!cgrp->name)
> 		goto err_free_cgrp;
> 
> The reason that this is bad practice is that it is hiding the fact that
> the allocation and initialization in cgroup_alloc_name() needs to be
> ordered before the assignment to cgrp->name.
> 
> Make sense?

Absolutely! But e.g. the pointer could be preallocated and no length
checks are needed so there is no need for an error path, I guess someone
could think it is safe to put the get_ptr_with_side_effects (and if the
side effect is only a bit flip in the strucutre pointed to by the value)
in the v argument of rcu_assign_pointer.

I also tought about adding a (*(&(v))) statement to enfore that only
lvalues where allowed as the value in rcu_assign_pointer, but that would
already cause compilation errors (e.g. rcu_assign_pointer(ptr, ptr|MARK)).

The nice thing with ACCESS_ONCE(p) is, that there is now no way to put
a non-lvalue expression into the first argument of rcu_assign_pointer. :)

Regarding the volatile access, I hope that the C11 memory model
and enhancements to the compiler will some day provide a better
way to express the semantics of what is tried to express here
(__atomic_store_n/__atomic_load_n with the accompanied memory model,
which could be even weaker to what a volatile access would enfore
now and could guarantee atomic stores/loads).

Greeting,

  Hannes

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Hannes Frederic Sowa]

On Sat, Oct 12, 2013 at 06:43:45PM +0200, Hannes Frederic Sowa wrote:
> Regarding the volatile access, I hope that the C11 memory model
> and enhancements to the compiler will some day provide a better
> way to express the semantics of what is tried to express here
> (__atomic_store_n/__atomic_load_n with the accompanied memory model,
> which could be even weaker to what a volatile access would enfore
> now and could guarantee atomic stores/loads).

I just played around a bit more. Perhaps we could try to warn of silly
usages of ACCESS_ONCE():

--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -349,7 +349,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  * use is to mediate communication between process-level code and irq/NMI
  * handlers, all running on the same CPU.
  */
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#define ACCESS_ONCE(x) (*({						\
+		compiletime_assert(sizeof(typeof(x)) <= sizeof(typeof(&x)), \
+				   "ACCESS_ONCE likely not atomic");	\
+		(volatile typeof(x) *)&(x);				\
+}))
 
 /* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
 #ifdef CONFIG_KPROBES

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Mathieu Desnoyers]

----- Original Message -----
> From: "Hannes Frederic Sowa" <hannes@stressinduktion.org>
> To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, "Eric Dumazet" <eric.dumazet@gmail.com>, "Josh Triplett"
> <josh@joshtriplett.org>, linux-kernel@vger.kernel.org, mingo@kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com,
> akpm@linux-foundation.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>, niv@us.ibm.com, tglx@linutronix.de,
> peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, darren@dvhart.com,
> fweisbec@gmail.com, sbw@mit.edu, "David S. Miller" <davem@davemloft.net>, "Alexey Kuznetsov" <kuznet@ms2.inr.ac.ru>,
> "James Morris" <jmorris@namei.org>, "Hideaki YOSHIFUJI" <yoshfuji@linux-ipv6.org>, "Patrick McHardy"
> <kaber@trash.net>, netdev@vger.kernel.org
> Sent: Saturday, October 12, 2013 1:37:34 PM
> Subject: Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive
> 
> On Sat, Oct 12, 2013 at 06:43:45PM +0200, Hannes Frederic Sowa wrote:
> > Regarding the volatile access, I hope that the C11 memory model
> > and enhancements to the compiler will some day provide a better
> > way to express the semantics of what is tried to express here
> > (__atomic_store_n/__atomic_load_n with the accompanied memory model,
> > which could be even weaker to what a volatile access would enfore
> > now and could guarantee atomic stores/loads).
> 
> I just played around a bit more. Perhaps we could try to warn of silly
> usages of ACCESS_ONCE():
> 
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -349,7 +349,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> int val, int expect);
>   * use is to mediate communication between process-level code and irq/NMI
>   * handlers, all running on the same CPU.
>   */
> -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> +#define ACCESS_ONCE(x) (*({						\
> +		compiletime_assert(sizeof(typeof(x)) <= sizeof(typeof(&x)), \
> +				   "ACCESS_ONCE likely not atomic");	\

AFAIU, ACCESS_ONCE() is not meant to ensure atomicity of load/store, but rather merely ensures that the compiler will not merge nor refetch accesses. I don't think the assert check you propose is appropriate with respect to the ACCESS_ONCE() semantic.

Thanks,

Mathieu

> +		(volatile typeof(x) *)&(x);				\
> +}))
>  
>  /* Ignore/forbid kprobes attach on very low level functions marked by this
>  attribute: */
>  #ifdef CONFIG_KPROBES
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Sat, Oct 12, 2013 at 07:42:18PM +0000, Mathieu Desnoyers wrote:
> ----- Original Message -----
> > On Sat, Oct 12, 2013 at 06:43:45PM +0200, Hannes Frederic Sowa wrote:
> > > Regarding the volatile access, I hope that the C11 memory model
> > > and enhancements to the compiler will some day provide a better
> > > way to express the semantics of what is tried to express here
> > > (__atomic_store_n/__atomic_load_n with the accompanied memory model,
> > > which could be even weaker to what a volatile access would enfore
> > > now and could guarantee atomic stores/loads).
> > 
> > I just played around a bit more. Perhaps we could try to warn of silly
> > usages of ACCESS_ONCE():
> > 
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -349,7 +349,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> > int val, int expect);
> >   * use is to mediate communication between process-level code and irq/NMI
> >   * handlers, all running on the same CPU.
> >   */
> > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > +#define ACCESS_ONCE(x) (*({						\
> > +		compiletime_assert(sizeof(typeof(x)) <= sizeof(typeof(&x)), \
> > +				   "ACCESS_ONCE likely not atomic");	\
> 
> AFAIU, ACCESS_ONCE() is not meant to ensure atomicity of load/store,
> but rather merely ensures that the compiler will not merge nor refetch
> accesses. I don't think the assert check you propose is appropriate with
> respect to the ACCESS_ONCE() semantic.

I am with Mathieu on this one, at least unless there is some set of actual
bugs already in the kernel that these length checks would find.

/me wonders about structs of size 3, 5, 6, and 7...

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> > +		(volatile typeof(x) *)&(x);				\
> > +}))
> >  
> >  /* Ignore/forbid kprobes attach on very low level functions marked by this
> >  attribute: */
> >  #ifdef CONFIG_KPROBES
> > 
> > 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Hannes Frederic Sowa]

On Sun, Oct 13, 2013 at 04:14:39AM -0700, Paul E. McKenney wrote:
> On Sat, Oct 12, 2013 at 07:42:18PM +0000, Mathieu Desnoyers wrote:
> > ----- Original Message -----
> > > On Sat, Oct 12, 2013 at 06:43:45PM +0200, Hannes Frederic Sowa wrote:
> > > > Regarding the volatile access, I hope that the C11 memory model
> > > > and enhancements to the compiler will some day provide a better
> > > > way to express the semantics of what is tried to express here
> > > > (__atomic_store_n/__atomic_load_n with the accompanied memory model,
> > > > which could be even weaker to what a volatile access would enfore
> > > > now and could guarantee atomic stores/loads).
> > > 
> > > I just played around a bit more. Perhaps we could try to warn of silly
> > > usages of ACCESS_ONCE():
> > > 
> > > --- a/include/linux/compiler.h
> > > +++ b/include/linux/compiler.h
> > > @@ -349,7 +349,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> > > int val, int expect);
> > >   * use is to mediate communication between process-level code and irq/NMI
> > >   * handlers, all running on the same CPU.
> > >   */
> > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > > +#define ACCESS_ONCE(x) (*({						\
> > > +		compiletime_assert(sizeof(typeof(x)) <= sizeof(typeof(&x)), \
> > > +				   "ACCESS_ONCE likely not atomic");	\
> > 
> > AFAIU, ACCESS_ONCE() is not meant to ensure atomicity of load/store,
> > but rather merely ensures that the compiler will not merge nor refetch
> > accesses. I don't think the assert check you propose is appropriate with
> > respect to the ACCESS_ONCE() semantic.
> 
> I am with Mathieu on this one, at least unless there is some set of actual
> bugs already in the kernel that these length checks would find.

I guess my wording of "ACCESS_ONCE likely not atomic" was misplaced. Something
like volatile access to memory larger than the processor register size is
probably not what you intended. Use atomics or proper locking. ;)
And maybe that is not even correct.

> /me wonders about structs of size 3, 5, 6, and 7...

Checked a x86_64 allyesconfig build with sizes above pointer size and odd
parity and nothing broke.

Greetings,

  Hannes

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Josh Triplett]

On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > 
> > > that.  Constructs like list_del_rcu are much clearer, and not
> > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > Idea.
> > 
> > OK, so you think there is synchronization code.
> > 
> > I will shut up then, no need to waste time.
> 
> As you said earlier, we should at least get rid of the memory barrier
> as long as we are changing the code.
> 
> Josh, what would you suggest as the best way to avoid the memory barrier,
> keep sparse happy, and not be too ugly?

The more I think about it, the more I realize that assigning an __rcu
pointer to an __rcu pointer *without* a memory barrier is a sufficiently
uncommon case that you probably *should* just write an open-coded
assignment.  Just please put a very clear comment right before it.

I'd originally thought it might make sense to have a macro similar to
rcu_assign_pointer, but I just don't think this is a common enough case,
and we don't want people thinking they can use this in general for __rcu
to __rcu assignments (most of which still need a memory barrier).

- Josh Triplett

Re: [PATCH v2 tip/core/rcu 07/13] ipv6/ip6_tunnel: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

On Thu, Oct 10, 2013 at 05:20:44PM -0700, Josh Triplett wrote:
> On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote:
> > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote:
> > > 
> > > > that.  Constructs like list_del_rcu are much clearer, and not
> > > > open-coded.  Open-coding synchronization code is almost always a Bad
> > > > Idea.
> > > 
> > > OK, so you think there is synchronization code.
> > > 
> > > I will shut up then, no need to waste time.
> > 
> > As you said earlier, we should at least get rid of the memory barrier
> > as long as we are changing the code.
> > 
> > Josh, what would you suggest as the best way to avoid the memory barrier,
> > keep sparse happy, and not be too ugly?
> 
> The more I think about it, the more I realize that assigning an __rcu
> pointer to an __rcu pointer *without* a memory barrier is a sufficiently
> uncommon case that you probably *should* just write an open-coded
> assignment.  Just please put a very clear comment right before it.

Fair enough, will do!  Given earlier email, I believe that Eric is
fine with this, and if he isn't I am sure he will let us know.  ;-)

> I'd originally thought it might make sense to have a macro similar to
> rcu_assign_pointer, but I just don't think this is a common enough case,
> and we don't want people thinking they can use this in general for __rcu
> to __rcu assignments (most of which still need a memory barrier).

Yep, it is a rather small fraction of rcu_assign_pointer() instances.

							Thanx, Paul

[PATCH v2 tip/core/rcu 08/13] ipv6/ip6_gre: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
ip6gre_tunnel_unlink() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/ip6_gre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 6b26e9feafb9..a0b8f4056f05 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -276,7 +276,7 @@ static void ip6gre_tunnel_unlink(struct ip6gre_net *ign, struct ip6_tnl *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			rcu_assign_pointer(*tp, rcu_access_pointer(t->next));
 			break;
 		}
 	}
-- 
1.8.1.5

[PATCH v2 tip/core/rcu 09/13] ipv6/sit: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
ipip6_tunnel_unlink() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/sit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 7ee5cb96db34..fcb050ab5134 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -157,7 +157,7 @@ static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			rcu_assign_pointer(*tp, rcu_access_pointer(t->next));
 			break;
 		}
 	}
-- 
1.8.1.5

[PATCH v2 tip/core/rcu 10/13] mac80211: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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
sta_info_hash_del() 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 this false positive by laundering the
pointer 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: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 net/mac80211/sta_info.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aeb967a0aeed..d18ab89a5725 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -75,7 +75,7 @@ static int sta_info_hash_del(struct ieee80211_local *local,
 		return -ENOENT;
 	if (s == sta) {
 		rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)],
-				   s->hnext);
+				   rcu_access_pointer(s->hnext));
 		return 0;
 	}
 
@@ -84,7 +84,7 @@ static int sta_info_hash_del(struct ieee80211_local *local,
 		s = rcu_dereference_protected(s->hnext,
 					lockdep_is_held(&local->sta_mtx));
 	if (rcu_access_pointer(s->hnext)) {
-		rcu_assign_pointer(s->hnext, sta->hnext);
+		rcu_assign_pointer(s->hnext, rcu_access_pointer(sta->hnext));
 		return 0;
 	}
 
-- 
1.8.1.5

[PATCH v2 tip/core/rcu 11/13] bridge/br_mdb: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 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@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/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

[PATCH v2 tip/core/rcu 12/13] bonding/bond_main: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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
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@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
---
 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

[PATCH v2 tip/core/rcu 13/13] bonding/bond_alb.c: Apply rcu_access_pointer() to avoid sparse false positive

[Paul E. McKenney]

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 use in
bond_alb_handle_active_change() is legitimate: It is 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 this false positive by laundering the
pointer 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: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 drivers/net/bonding/bond_alb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 91f179d5135c..cdd697cca6b5 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1667,7 +1667,8 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
 	}
 
 	swap_slave = bond->curr_active_slave;
-	rcu_assign_pointer(bond->curr_active_slave, new_slave);
+	rcu_assign_pointer(bond->curr_active_slave,
+			   rcu_access_pointer(new_slave));
 
 	if (!new_slave || list_empty(&bond->slave_list))
 		return;
-- 
1.8.1.5

Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[Josh Triplett]

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

Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[Paul E. McKenney]

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

Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[Josh Triplett]

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

Re: [PATCH v2 tip/core/rcu 0/13] Sparse-related updates for 3.13

[Josh Triplett]

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