diff mbox

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

Message ID 1381354186-16285-7-git-send-email-paulmck@linux.vnet.ibm.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Paul E. McKenney Oct. 9, 2013, 9:29 p.m. UTC 
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(-)

Comments

Eric Dumazet Oct. 9, 2013, 9:42 p.m. UTC | #1 
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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul E. McKenney Oct. 9, 2013, 9:57 p.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Oct. 9, 2013, 10:10 p.m. UTC | #3 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul E. McKenney Oct. 9, 2013, 10:36 p.m. UTC | #4 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 		}
 	}