lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 22 Dec 2014 16:30:40 +0800 From: Wengang <wen.gang.wang@...cle.com> To: Ding Tianhong <dingtianhong@...wei.com>, Andy Gospodarek <gospo@...ulusnetworks.com> CC: netdev@...r.kernel.org Subject: Re: [PATCH] bonding: avoid re-entry of bond_release OK. Will change as suggested and re-post. thanks, wengang 于 2014年12月22日 10:05, Ding Tianhong 写道: > On 2014/12/22 9:09, Wengang wrote: >> Hi Andy and Ding, >> >> Thanks for your reviews! >> In the ioctl path, removing a interface that is not currently actually a slave >> can happen from user space(by mistake), we should avoid the noisy message. >> >> While, __bond_release_one() has another call path which is from bond_uninit(). >> In the later case, it should be treated as an error if the interface is not with >> IFF_SLAVE flag. To notice that error occurred, the message is printed. I think >> the message is needed for this path. >> >> How do you think? >> > Just like the bond_enslave(), it is only a warning. > > Ding > >> thanks, >> wengang >> >> 于 2014年12月21日 10:01, Ding Tianhong 写道: >>> On 2014/12/19 23:11, Andy Gospodarek wrote: >>>> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote: >>>>> If bond_release is run against an interface which is already detached from >>>>> it's master, then there is an error message shown like >>>>> "<master name> cannot release <slave name>". >>>>> >>>>> The call path is: >>>>> bond_do_ioctl() >>>>> bond_release() >>>>> __bond_release_one() >>>>> >>>>> Though it does not really harm, the message the message is misleading. >>>>> This patch tries to avoid the message. >>>>> >>>>> Signed-off-by: Wengang Wang <wen.gang.wang@...cle.com> >>>>> --- >>>>> drivers/net/bonding/bond_main.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >>>>> index 184c434..4a71bbd 100644 >>>>> --- a/drivers/net/bonding/bond_main.c >>>>> +++ b/drivers/net/bonding/bond_main.c >>>>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd >>>>> break; >>>>> case BOND_RELEASE_OLD: >>>>> case SIOCBONDRELEASE: >>>>> - res = bond_release(bond_dev, slave_dev); >>>>> + if (slave_dev->flags & IFF_SLAVE) >>>>> + res = bond_release(bond_dev, slave_dev); >>>>> + else >>>>> + res = 0; >>>> Functionally this patch is fine, but I would prefer that you simply >>>> change the check in __bond_release_one to not be so noisy. There is a >>>> check[1] in bond_enslave to see if a slave is already in a bond and that >>>> just prints a message of netdev_dbg (rather than netdev_err) and it >>>> seems that would be appropriate for this type of message. >>>> >>>> [1] from bond_enslave(): >>>> >>>> /* already enslaved */ >>>> if (slave_dev->flags & IFF_SLAVE) { >>>> netdev_dbg(bond_dev, "Error: Device was already enslaved\n"); >>>> return -EBUSY; >>>> } >>>> >>>> >>>>> break; >>>>> case BOND_SETHWADDR_OLD: >>>>> case SIOCBONDSETHWADDR: >>>>> -- >>> agree ,use netdev_dbg looks more better and enough. >>> >>> Ding >>> >>> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists