[PATCH v2 net-next 0/3] bonding: fix bond_3ad RCU usage

[PATCH v2 net-next 0/3] bonding: fix bond_3ad RCU usage

[Veaceslav Falico]

While digging through bond_3ad.c I've found that the RCU usage there is
just wrong - it's used as a kind of mutex/spinlock instead of RCU.

v1->v2: use generic primitives instead of _rcu ones cause we can hold RTNL
lock without RCU one, which is still safe.

This patchset is on top of bond_3ad.c cleanup:
http://www.spinics.net/lists/netdev/msg265447.html

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

[PATCH v2 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage

[Veaceslav Falico]

Currently, its usage is just plainly wrong. It first gets a slave under
RCU, and, after releasing the RCU lock, continues to use it - whilst it can
be freed.

Fix this by ensuring that bond_3ad_set_carrier() is either under RCU read
lock or under the RTNL lock (bond_set_carrier() always holds it).

Fixes: be79bd048ab ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

Notes:
    v1 -> v2:
    Don't use _rcu primitives as we can be called under RTNL too.

 drivers/net/bonding/bond_3ad.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 29db1ca..cf5fab8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1597,9 +1597,9 @@ static void ad_agg_selection_logic(struct aggregator *agg)
 		}
 	}
 
-	rcu_read_unlock();
-
 	bond_3ad_set_carrier(bond);
+
+	rcu_read_unlock();
 }
 
 /**
@@ -2322,15 +2322,14 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
  *
  * Called by bond_set_carrier(). Return zero if carrier state does not
  * change, nonzero if it does.
+ * The caller must hold RCU read lock or RTNL.
  */
 int bond_3ad_set_carrier(struct bonding *bond)
 {
 	struct aggregator *active;
 	struct slave *first_slave;
 
-	rcu_read_lock();
-	first_slave = bond_first_slave_rcu(bond);
-	rcu_read_unlock();
+	first_slave = bond_first_slave(bond);
 	if (!first_slave)
 		return 0;
 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
-- 
1.8.4

Re: [PATCH v2 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage

[Ding Tianhong]

On 2014/1/9 19:20, Veaceslav Falico wrote:
> Currently, its usage is just plainly wrong. It first gets a slave under
> RCU, and, after releasing the RCU lock, continues to use it - whilst it can
> be freed.
> 
> Fix this by ensuring that bond_3ad_set_carrier() is either under RCU read
> lock or under the RTNL lock (bond_set_carrier() always holds it).
> 
> Fixes: be79bd048ab ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
> 
> Notes:
>     v1 -> v2:
>     Don't use _rcu primitives as we can be called under RTNL too.
> 
>  drivers/net/bonding/bond_3ad.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 29db1ca..cf5fab8 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -1597,9 +1597,9 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>  		}
>  	}
>  
> -	rcu_read_unlock();
> -
>  	bond_3ad_set_carrier(bond);
> +
> +	rcu_read_unlock();
>  }
>  
>  /**
> @@ -2322,15 +2322,14 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>   *
>   * Called by bond_set_carrier(). Return zero if carrier state does not
>   * change, nonzero if it does.
> + * The caller must hold RCU read lock or RTNL.
>   */
>  int bond_3ad_set_carrier(struct bonding *bond)
>  {
>  	struct aggregator *active;
>  	struct slave *first_slave;
>  
> -	rcu_read_lock();
> -	first_slave = bond_first_slave_rcu(bond);
> -	rcu_read_unlock();
> +	first_slave = bond_first_slave(bond);
>  	if (!first_slave)
>  		return 0;
>  	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
> 

Agree, thanks.

Acked-by: Ding Tianhong <dingtianhong@huawei.com>

[PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage

[Veaceslav Falico]

Currently, the RCU read lock usage is just wrong - it gets the slave struct
under RCU and continues to use it when RCU lock is released.

However, it's still safe to do this cause we didn't need the
rcu_read_lock() initially - all of the __get_first_agg() callers are either
holding RCU read lock or the RTNL lock, so that we can't sync while in it.

So, remove the useless rcu locking and add a comment.

Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

Notes:
    v1 -> v2:
    Don't use RCU primitives as we can hold RTNL.

 drivers/net/bonding/bond_3ad.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index cf5fab8..d2782c8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -143,6 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
  *
  * Return the aggregator of the first slave in @bond, or %NULL if it can't be
  * found.
+ * The caller must either hold RCU or RTNL lock.
  */
 static inline struct aggregator *__get_first_agg(struct port *port)
 {
@@ -153,9 +154,7 @@ static inline struct aggregator *__get_first_agg(struct port *port)
 	if (bond == NULL)
 		return NULL;
 
-	rcu_read_lock();
-	first_slave = bond_first_slave_rcu(bond);
-	rcu_read_unlock();
+	first_slave = bond_first_slave(bond);
 
 	return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
 }
-- 
1.8.4

Re: [PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage

[Ding Tianhong]

On 2014/1/9 19:20, Veaceslav Falico wrote:
> Currently, the RCU read lock usage is just wrong - it gets the slave struct
> under RCU and continues to use it when RCU lock is released.
> 
> However, it's still safe to do this cause we didn't need the
> rcu_read_lock() initially - all of the __get_first_agg() callers are either
> holding RCU read lock or the RTNL lock, so that we can't sync while in it.
> 
> So, remove the useless rcu locking and add a comment.
> 
> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
> 
> Notes:
>     v1 -> v2:
>     Don't use RCU primitives as we can hold RTNL.
> 
>  drivers/net/bonding/bond_3ad.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index cf5fab8..d2782c8 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -143,6 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
>   *
>   * Return the aggregator of the first slave in @bond, or %NULL if it can't be
>   * found.
> + * The caller must either hold RCU or RTNL lock.
>   */
>  static inline struct aggregator *__get_first_agg(struct port *port)
>  {
> @@ -153,9 +154,7 @@ static inline struct aggregator *__get_first_agg(struct port *port)
>  	if (bond == NULL)
>  		return NULL;
>  
> -	rcu_read_lock();
> -	first_slave = bond_first_slave_rcu(bond);
> -	rcu_read_unlock();
> +	first_slave = bond_first_slave(bond);
>  
>  	return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
>  }
> 

Hi, Veaceslav:

Do you mean the bond_first_slave is safe in rcu_read_xxlock()?

#define bond_first_slave(bond) \
	(bond_has_slaves(bond) ? \
		netdev_adjacent_get_private(bond_slave_list(bond)->next) : \
		NULL)


Regards
Ding

Re: [PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage

[Veaceslav Falico]

On Thu, Jan 09, 2014 at 07:58:17PM +0800, Ding Tianhong wrote:
>On 2014/1/9 19:20, Veaceslav Falico wrote:
>> Currently, the RCU read lock usage is just wrong - it gets the slave struct
>> under RCU and continues to use it when RCU lock is released.
>>
>> However, it's still safe to do this cause we didn't need the
>> rcu_read_lock() initially - all of the __get_first_agg() callers are either
>> holding RCU read lock or the RTNL lock, so that we can't sync while in it.
>>
>> So, remove the useless rcu locking and add a comment.
>>
>> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
>> CC: dingtianhong@huawei.com
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>
>> Notes:
>>     v1 -> v2:
>>     Don't use RCU primitives as we can hold RTNL.
>>
>>  drivers/net/bonding/bond_3ad.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index cf5fab8..d2782c8 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -143,6 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
>>   *
>>   * Return the aggregator of the first slave in @bond, or %NULL if it can't be
>>   * found.
>> + * The caller must either hold RCU or RTNL lock.
>>   */
>>  static inline struct aggregator *__get_first_agg(struct port *port)
>>  {
>> @@ -153,9 +154,7 @@ static inline struct aggregator *__get_first_agg(struct port *port)
>>  	if (bond == NULL)
>>  		return NULL;
>>
>> -	rcu_read_lock();
>> -	first_slave = bond_first_slave_rcu(bond);
>> -	rcu_read_unlock();
>> +	first_slave = bond_first_slave(bond);
>>
>>  	return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
>>  }
>>
>
>Hi, Veaceslav:
>
>Do you mean the bond_first_slave is safe in rcu_read_xxlock()?
>
>#define bond_first_slave(bond) \
>	(bond_has_slaves(bond) ? \
>		netdev_adjacent_get_private(bond_slave_list(bond)->next) : \
>		NULL)

Heh, good catch. We can't use neither _rcu primitives, because we can be
called under RTNL, and nor the RTNL ones, cause we can be called under RCU.
Not that easy to fix your RCU updates...

I'll take a look and send v3.

>
>
>Regards
>Ding
>
>

[PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic

[Veaceslav Falico]

Currently, the implementation is meaningless - once again, we take the
slave structure and use it after we've exited RCU critical section.

Fix this by removing the rcu_read_lock() from __get_active_agg(), and
ensuring that all its callers are holding either RCU or RTNL lock.

Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

Notes:
    v1 -> v2:
    Don't use RCU primitives as we can hold RTNL.

 drivers/net/bonding/bond_3ad.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index d2782c8..845545b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -674,6 +674,8 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
 /**
  * __get_active_agg - get the current active aggregator
  * @aggregator: the aggregator we're looking at
+ *
+ * Caller must hold either RCU or RTNL lock.
  */
 static struct aggregator *__get_active_agg(struct aggregator *aggregator)
 {
@@ -681,13 +683,9 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
 	struct list_head *iter;
 	struct slave *slave;
 
-	rcu_read_lock();
-	bond_for_each_slave_rcu(bond, slave, iter)
-		if (SLAVE_AD_INFO(slave).aggregator.is_active) {
-			rcu_read_unlock();
+	bond_for_each_slave(bond, slave, iter)
+		if (SLAVE_AD_INFO(slave).aggregator.is_active)
 			return &(SLAVE_AD_INFO(slave).aggregator);
-		}
-	rcu_read_unlock();
 
 	return NULL;
 }
@@ -1495,11 +1493,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
 	struct slave *slave;
 	struct port *port;
 
+	rcu_read_lock();
 	origin = agg;
 	active = __get_active_agg(agg);
 	best = (active && agg_device_up(active)) ? active : NULL;
 
-	rcu_read_lock();
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		agg = &(SLAVE_AD_INFO(slave).aggregator);
 
-- 
1.8.4

Re: [PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic

[Ding Tianhong]

On 2014/1/9 19:20, Veaceslav Falico wrote:
> Currently, the implementation is meaningless - once again, we take the
> slave structure and use it after we've exited RCU critical section.
> 
> Fix this by removing the rcu_read_lock() from __get_active_agg(), and
> ensuring that all its callers are holding either RCU or RTNL lock.
> 
> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
> 
> Notes:
>     v1 -> v2:
>     Don't use RCU primitives as we can hold RTNL.
> 
>  drivers/net/bonding/bond_3ad.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index d2782c8..845545b 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -674,6 +674,8 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>  /**
>   * __get_active_agg - get the current active aggregator
>   * @aggregator: the aggregator we're looking at
> + *
> + * Caller must hold either RCU or RTNL lock.
>   */
>  static struct aggregator *__get_active_agg(struct aggregator *aggregator)
>  {
> @@ -681,13 +683,9 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
>  	struct list_head *iter;
>  	struct slave *slave;
>  
> -	rcu_read_lock();
> -	bond_for_each_slave_rcu(bond, slave, iter)
> -		if (SLAVE_AD_INFO(slave).aggregator.is_active) {
> -			rcu_read_unlock();
> +	bond_for_each_slave(bond, slave, iter)
> +		if (SLAVE_AD_INFO(slave).aggregator.is_active)
>  			return &(SLAVE_AD_INFO(slave).aggregator);
> -		}

Is the bond_for_each_slave safe in rcu_read_lock()?


> -	rcu_read_unlock();
>  
>  	return NULL;
>  }
> @@ -1495,11 +1493,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>  	struct slave *slave;
>  	struct port *port;
>  
> +	rcu_read_lock();
>  	origin = agg;
>  	active = __get_active_agg(agg);
>  	best = (active && agg_device_up(active)) ? active : NULL;
>  
> -	rcu_read_lock();
>  	bond_for_each_slave_rcu(bond, slave, iter) {
>  		agg = &(SLAVE_AD_INFO(slave).aggregator);
>  
> 

Re: [PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic

[Veaceslav Falico]

On Thu, Jan 09, 2014 at 08:04:25PM +0800, Ding Tianhong wrote:
>On 2014/1/9 19:20, Veaceslav Falico wrote:
>> Currently, the implementation is meaningless - once again, we take the
>> slave structure and use it after we've exited RCU critical section.
>>
>> Fix this by removing the rcu_read_lock() from __get_active_agg(), and
>> ensuring that all its callers are holding either RCU or RTNL lock.
>>
>> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
>> CC: dingtianhong@huawei.com
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>
>> Notes:
>>     v1 -> v2:
>>     Don't use RCU primitives as we can hold RTNL.
>>
>>  drivers/net/bonding/bond_3ad.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index d2782c8..845545b 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -674,6 +674,8 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>>  /**
>>   * __get_active_agg - get the current active aggregator
>>   * @aggregator: the aggregator we're looking at
>> + *
>> + * Caller must hold either RCU or RTNL lock.
>>   */
>>  static struct aggregator *__get_active_agg(struct aggregator *aggregator)
>>  {
>> @@ -681,13 +683,9 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
>>  	struct list_head *iter;
>>  	struct slave *slave;
>>
>> -	rcu_read_lock();
>> -	bond_for_each_slave_rcu(bond, slave, iter)
>> -		if (SLAVE_AD_INFO(slave).aggregator.is_active) {
>> -			rcu_read_unlock();
>> +	bond_for_each_slave(bond, slave, iter)
>> +		if (SLAVE_AD_INFO(slave).aggregator.is_active)
>>  			return &(SLAVE_AD_INFO(slave).aggregator);
>> -		}
>
>Is the bond_for_each_slave safe in rcu_read_lock()?

Yeah, good catch, responded in the second patch.

Thanks for the review!

>
>
>> -	rcu_read_unlock();
>>
>>  	return NULL;
>>  }
>> @@ -1495,11 +1493,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>>  	struct slave *slave;
>>  	struct port *port;
>>
>> +	rcu_read_lock();
>>  	origin = agg;
>>  	active = __get_active_agg(agg);
>>  	best = (active && agg_device_up(active)) ? active : NULL;
>>
>> -	rcu_read_lock();
>>  	bond_for_each_slave_rcu(bond, slave, iter) {
>>  		agg = &(SLAVE_AD_INFO(slave).aggregator);
>>
>>
>
>