Re: [3/4] bonding: the calling of bond->slave_cnt need protection

[Ding Tianhong <dingtianhong@huawei.com>]

On 2013/7/20 18:47, Veaceslav Falico wrote:
> On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote:
>> The bonding_store_mode has rtnl protection, so no need to get read lock
>> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect
>> the bond->slave_cnt, so add read_lock().
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>
>> ---
>> drivers/net/bonding/bond_sysfs.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>> index dc36a3d..d01a189 100644
>> --- a/drivers/net/bonding/bond_sysfs.c
>> +++ b/drivers/net/bonding/bond_sysfs.c
>> @@ -504,11 +504,14 @@ static ssize_t bonding_store_fail_over_mac(struct device *d,
>>     int new_value;
>>     struct bonding *bond = to_bond(d);
>>
>> +    read_lock(&bond->lock);
>>     if (bond->slave_cnt != 0) {
>>         pr_err("%s: Can't alter fail_over_mac with slaves in bond.\n",
>>                bond->dev->name);
>> +        read_unlock(&bond->lock);
>>         return -EPERM;
>>     }
>> +    read_unlock(&bond->lock);
> 
> Maybe it's Saturday, but I really don't see *any* point in this locking.
> 
> I think you've meant that we need the rtnl protection while reading
> slave_cnt AND updating the .fail_over_mac, so that in between we won't add
> new slaves with outdated params.

yes, as you said, the rtnl lock is enough here, but I think rtnl lock is bigger than 
a readlock, so think about the performance, i choose the readlock to protect slave_cnt.

> 
> Something like this (untested):
> 
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index dc36a3d..8a5a6a3 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -501,20 +501,25 @@ static ssize_t bonding_store_fail_over_mac(struct device *d,
>                         struct device_attribute *attr,
>                         const char *buf, size_t count)
>  {
> -    int new_value;
> +    int new_value, ret = count;
>      struct bonding *bond = to_bond(d);
>  
> +    if (!rtnl_trylock())
> +        return restart_syscall();
> +
>      if (bond->slave_cnt != 0) {
>          pr_err("%s: Can't alter fail_over_mac with slaves in bond.\n",
>                 bond->dev->name);
> -        return -EPERM;
> +        ret = -EPERM;
> +        goto out;
>      }
>  
>      new_value = bond_parse_parm(buf, fail_over_mac_tbl);
>      if (new_value < 0) {
>          pr_err("%s: Ignoring invalid fail_over_mac value %s.\n",
>                 bond->dev->name, buf);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
>      bond->params.fail_over_mac = new_value;
> @@ -522,7 +527,9 @@ static ssize_t bonding_store_fail_over_mac(struct device *d,
>          bond->dev->name, fail_over_mac_tbl[new_value].modename,
>          new_value);
>  
> -    return count;
> +out:
> +    rtnl_unlock();
> +    return ret;
>  }
>  
>  static DEVICE_ATTR(fail_over_mac, S_IRUGO | S_IWUSR,
>>
>>     new_value = bond_parse_parm(buf, fail_over_mac_tbl);
>>     if (new_value < 0) {
> 
> .
>