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

login
register
mail settings
Submitter Veaceslav Falico
Date July 20, 2013, 10:47 a.m.
Message ID <20130720104746.GC9149@redhat.com>
Download mbox | patch
Permalink /patch/260438/
State RFC
Delegated to: David Miller
Headers show

Comments

Veaceslav Falico - July 20, 2013, 10:47 a.m.
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.

Something like this (untested):

>
> 	new_value = bond_parse_parm(buf, fail_over_mac_tbl);
> 	if (new_value < 0) {
--
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
Nikolay Aleksandrov - July 20, 2013, 12:42 p.m.
On 07/20/2013 12:47 PM, 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>
>>
<snip>
> 
> 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.
> 
> Something like this (untested):
> 
Indeed, Veaceslav's way is the correct one (I've looked at this race
before), but IMO it's not worth it to protect fail_over_mac as the worst
that could happen is inconsistency with the MAC addresses which isn't
fatal. Anyway, I still haven't had my coffee and might be missing something :-)

Cheers,
 Nik

--
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
Veaceslav Falico - July 20, 2013, 3 p.m.
On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote:
>On 07/20/2013 12:47 PM, 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>
>>>
><snip>
>>
>> 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.
>>
>> Something like this (untested):
>>
>Indeed, Veaceslav's way is the correct one (I've looked at this race
>before), but IMO it's not worth it to protect fail_over_mac as the worst
>that could happen is inconsistency with the MAC addresses which isn't
>fatal. Anyway, I still haven't had my coffee and might be missing something :-)

Yep, agree that it's kind of minor and hard to hit in real life.

OTOH, getting the rtnl here costs us virtually nothing and might save
someone from a headache :). And it also follows the logic "don't change
anything slave-related without rtnl".

So I'd rather have it, as a minor improvement :).

>
>Cheers,
> Nik
>
--
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
dingtianhong - July 22, 2013, 12:42 a.m.
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) {
> 
> .
> 


--
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
dingtianhong - July 22, 2013, 12:47 a.m.
On 2013/7/20 23:00, Veaceslav Falico wrote:
> On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote:
>> On 07/20/2013 12:47 PM, 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>
>>>>
>> <snip>
>>>
>>> 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.
>>>
>>> Something like this (untested):
>>>
>> Indeed, Veaceslav's way is the correct one (I've looked at this race
>> before), but IMO it's not worth it to protect fail_over_mac as the worst
>> that could happen is inconsistency with the MAC addresses which isn't
>> fatal. Anyway, I still haven't had my coffee and might be missing something :-)
> 
> Yep, agree that it's kind of minor and hard to hit in real life.
> 
> OTOH, getting the rtnl here costs us virtually nothing and might save
> someone from a headache :). And it also follows the logic "don't change
> anything slave-related without rtnl".
> 
> So I'd rather have it, as a minor improvement :).
> 
>>
>> Cheers,
>> Nik

yes , i think it is hard to hit the problem in real lift, just looks better. :)

>>
> 
> 


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

Patch

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,