netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3/4] bonding: the calling of bond->slave_cnt need protection
@ 2013-07-20  7:23 Ding Tianhong
  2013-07-20 10:47 ` [3/4] " Veaceslav Falico
  0 siblings, 1 reply; 6+ messages in thread
From: Ding Tianhong @ 2013-07-20  7:23 UTC (permalink / raw)
  To: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev

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);
 
 	new_value = bond_parse_parm(buf, fail_over_mac_tbl);
 	if (new_value < 0) {
-- 
1.8.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [3/4] bonding: the calling of bond->slave_cnt need protection
  2013-07-20  7:23 [PATCH 3/4] bonding: the calling of bond->slave_cnt need protection Ding Tianhong
@ 2013-07-20 10:47 ` Veaceslav Falico
  2013-07-20 12:42   ` Nikolay Aleksandrov
  2013-07-22  0:42   ` Ding Tianhong
  0 siblings, 2 replies; 6+ messages in thread
From: Veaceslav Falico @ 2013-07-20 10:47 UTC (permalink / raw)
  To: dingtianhong; +Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev

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

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [3/4] bonding: the calling of bond->slave_cnt need protection
  2013-07-20 10:47 ` [3/4] " Veaceslav Falico
@ 2013-07-20 12:42   ` Nikolay Aleksandrov
  2013-07-20 15:00     ` Veaceslav Falico
  2013-07-22  0:42   ` Ding Tianhong
  1 sibling, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-07-20 12:42 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: dingtianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [3/4] bonding: the calling of bond->slave_cnt need protection
  2013-07-20 12:42   ` Nikolay Aleksandrov
@ 2013-07-20 15:00     ` Veaceslav Falico
  2013-07-22  0:47       ` Ding Tianhong
  0 siblings, 1 reply; 6+ messages in thread
From: Veaceslav Falico @ 2013-07-20 15:00 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: dingtianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [3/4] bonding: the calling of bond->slave_cnt need protection
  2013-07-20 10:47 ` [3/4] " Veaceslav Falico
  2013-07-20 12:42   ` Nikolay Aleksandrov
@ 2013-07-22  0:42   ` Ding Tianhong
  1 sibling, 0 replies; 6+ messages in thread
From: Ding Tianhong @ 2013-07-22  0:42 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Netdev

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) {
> 
> .
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [3/4] bonding: the calling of bond->slave_cnt need protection
  2013-07-20 15:00     ` Veaceslav Falico
@ 2013-07-22  0:47       ` Ding Tianhong
  0 siblings, 0 replies; 6+ messages in thread
From: Ding Tianhong @ 2013-07-22  0:47 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: Nikolay Aleksandrov, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Netdev

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. :)

>>
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-22  0:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-20  7:23 [PATCH 3/4] bonding: the calling of bond->slave_cnt need protection Ding Tianhong
2013-07-20 10:47 ` [3/4] " Veaceslav Falico
2013-07-20 12:42   ` Nikolay Aleksandrov
2013-07-20 15:00     ` Veaceslav Falico
2013-07-22  0:47       ` Ding Tianhong
2013-07-22  0:42   ` Ding Tianhong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).