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

[Veaceslav Falico <vfalico@redhat.com>]

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