* [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).