netdev - Re: [PATCH] bonding: avoid re-entry of bond

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 22 Dec 2014 10:45:24 -0500
From:	Andy Gospodarek <gospo@...ulusnetworks.com>
To:	Wengang <wen.gang.wang@...cle.com>
Cc:	Ding Tianhong <dingtianhong@...wei.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] bonding: avoid re-entry of bond_release

On Mon, Dec 22, 2014 at 04:30:40PM +0800, Wengang wrote:
> OK. Will change as suggested and re-post.

Sounds great.  Thanks for your work on this.

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