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