Re: [PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-16 Thread Paul E. McKenney 
On Sat, Nov 16, 2013 at 12:32:16PM +0800, Ding Tianhong wrote:
> 于 2013/11/16 8:40, Paul E. McKenney 写道:
> > From: "Paul E. McKenney" 
> >
> > 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() 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.
> 
> I think it is fit for net-next.

Thank you!

If this is queued there, I would be happy to drop it from my tree.
There are no dependencies on anything in my tree.

Thanx, Paul

> > Reported-by: kbuild test robot 
> > Signed-off-by: Paul E. McKenney 
> > Cc: Stephen Hemminger 
> > Cc: "David S. Miller" 
> > Cc: bri...@lists.linux-foundation.org
> > Cc: net...@vger.kernel.org
> > ---
> >  drivers/net/bonding/bond_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c 
> > b/drivers/net/bonding/bond_main.c
> > index 72df399c4ab3..bbd7fd3e46fe 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) {
> > @@ -1801,7 +1802,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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-16 Thread Paul E. McKenney 
On Sat, Nov 16, 2013 at 12:32:16PM +0800, Ding Tianhong wrote:
 于 2013/11/16 8:40, Paul E. McKenney 写道:
  From: Paul E. McKenney paul...@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() 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.
 
 I think it is fit for net-next.

Thank you!

If this is queued there, I would be happy to drop it from my tree.
There are no dependencies on anything in my tree.

Thanx, Paul

  Reported-by: kbuild test robot fengguang...@intel.com
  Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
  Cc: Stephen Hemminger step...@networkplumber.org
  Cc: David S. Miller da...@davemloft.net
  Cc: bri...@lists.linux-foundation.org
  Cc: net...@vger.kernel.org
  ---
   drivers/net/bonding/bond_main.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/net/bonding/bond_main.c 
  b/drivers/net/bonding/bond_main.c
  index 72df399c4ab3..bbd7fd3e46fe 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) {
  @@ -1801,7 +1802,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
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-15 Thread Ding Tianhong 
于 2013/11/16 8:40, Paul E. McKenney 写道:
> From: "Paul E. McKenney" 
>
> 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() 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.
I think it is fit for net-next.


> Reported-by: kbuild test robot 
> Signed-off-by: Paul E. McKenney 
> Cc: Stephen Hemminger 
> Cc: "David S. Miller" 
> Cc: bri...@lists.linux-foundation.org
> Cc: net...@vger.kernel.org
> ---
>  drivers/net/bonding/bond_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 72df399c4ab3..bbd7fd3e46fe 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) {
> @@ -1801,7 +1802,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-15 Thread Paul E. McKenney 
From: "Paul E. McKenney" 

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() 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 
Signed-off-by: Paul E. McKenney 
Cc: Stephen Hemminger 
Cc: "David S. Miller" 
Cc: bri...@lists.linux-foundation.org
Cc: net...@vger.kernel.org
---
 drivers/net/bonding/bond_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 72df399c4ab3..bbd7fd3e46fe 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) {
@@ -1801,7 +1802,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-15 Thread Paul E. McKenney 
From: Paul E. McKenney paul...@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() 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...@intel.com
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Stephen Hemminger step...@networkplumber.org
Cc: David S. Miller da...@davemloft.net
Cc: bri...@lists.linux-foundation.org
Cc: net...@vger.kernel.org
---
 drivers/net/bonding/bond_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 72df399c4ab3..bbd7fd3e46fe 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) {
@@ -1801,7 +1802,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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

2013-11-15 Thread Ding Tianhong 
于 2013/11/16 8:40, Paul E. McKenney 写道:
 From: Paul E. McKenney paul...@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() 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.
I think it is fit for net-next.


 Reported-by: kbuild test robot fengguang...@intel.com
 Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
 Cc: Stephen Hemminger step...@networkplumber.org
 Cc: David S. Miller da...@davemloft.net
 Cc: bri...@lists.linux-foundation.org
 Cc: net...@vger.kernel.org
 ---
  drivers/net/bonding/bond_main.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

 diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
 index 72df399c4ab3..bbd7fd3e46fe 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) {
 @@ -1801,7 +1802,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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/