| 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
| ||
|
Message-ID: <73095d73b5d5470aa613201acceac3c4@huawei.com>
Date: Thu, 29 Jul 2021 01:46:46 +0000
From: "zhudi (J)" <zhudi21@...wei.com>
To: Jay Vosburgh <jay.vosburgh@...onical.com>
CC: "vfalico@...il.com" <vfalico@...il.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Chenxiang (EulerOS)" <rose.chen@...wei.com>
Subject: Re: [PATCH] bonding: Avoid adding slave devices to inactive bonding
> >> zhudi <zhudi21@...wei.com> wrote:
> >>
> >> >We need to refuse to add slave devices to the bonding which does
> >> >not set IFF_UP flag, otherwise some problems will be caused(such as
> >> >bond_set_carrier() will not sync carrier state to upper net device).
> >> >The ifenslave command can prevent such use case, but through the
> sysfs
> >> >interface, slave devices can still be added regardless of whether
> >> >the bonding is set with IFF_UP flag or not.
> >>
> >> What specifically happens in the carrier state issue you
> >> mention? Are there other specific issues?
> >
> >yes, The following steps can cause problems:
> > 1)bond0 is down
> > 2) ip link add link bond0 name ipvlan type ipvlan mode l2
> > 3) echo +enp2s7 >/sys/class/net/bond0/bonding/slaves
> > 4) ip link set bond0 up
> >
> > After these steps, use ip link command, we found ipvlan has NO-
> CARRIER:
> > ipvlan@...d0: <NO-CARRIER,
> BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state>
> >
> > This is because, bond_enslave()->bond_set_carrier()-
> >netif_carrier_on()...->netdev_state_change()
> >will not sync carrier state to ipvlan because the IFF_UP flag is not set.
>
> Correct, it's not supposed to at the time bond_enslave is called
> (because the bond is down at that time). Later, when the bond is set
> link up, NETDEV_UP and NETDEV_CHANGE netdevice events are issued, and
> devices logically stacked above the bond can receive a notification at
> that time. This is what, e.g., VLAN does, in vlan_device_event,
> ultimately by calling netif_stacked_transfer_operstate if I'm following
> the code correctly.
In fact, netif_carrier_on() has been called in bond_enslave(), so when we
set bond link up, only NETDEV_UP event will be issued(dev_change_flags()-> __dev_notify_flags()).
Unlike VLAN device, ipvlan device does not handle NETDEV_UP event.
The focus now is the ipvlan device needs to handle NETDEV_UP event.
>
> Looking at ipvlan, it does register for netdevice events
> (ipvlan_device_event) but doesn't appear to handle events for the device
> it is linked to.
>
> >> As far as I can recall, adding interfaces to the bond while the
> >> bond is down has worked for a very long time, so I'm concerned that
> >> disabling that functionality will have impact on existing
> >> configurations.
> >>
> >> Also, to the best of my knowledge, the currently packaged
> >> ifenslave programs are scripts that utilize the sysfs interface. I'm
> >> unaware of current usage of the old C ifenslave program (removed from
> >> the kernel source in 2013), although the kernel code should still
> >> support it.
> >
> > We still use old ifenslave command and it does only allow to add
> slave to bonding with up state, code is as follows:
> >
> > /* check if master is up; if not then fail any operation */
> > if (!(master_flags.ifr_flags & IFF_UP)) {
> > fprintf(stderr,
> > "Illegal operation; the specified master interface "
> > "'%s' is not up.\n",
> > master_ifname);
> > res = 1;
> > goto out;
> > }
> >
> > If so, the behavior of the new tool is inconsistent with that of the old
> tool.
>
> This restriction in ifenslave.c dates to 15-20 years ago, when
> (as I recall) the master->slave infrastructure in the kernel could not
> handle such linking while the interface was down. Another side effect
> at the time was that setting a bond down would release all slaves.
>
> In any event, the old ifenslave.c is not the standard by which
> we set the behavior of the driver. It is obsolete and no longer
> developed or distributed upstream.
I didn't track the change history of the bonding, thank you for your explanation.
>
> -J
>
> >> >So we introduce a new BOND_OPTFLAG_IFUP flag to avoid adding slave
> >> >devices to inactive bonding.
> >> >
> >> >Signed-off-by: zhudi <zhudi21@...wei.com>
> >> >---
> >> > drivers/net/bonding/bond_options.c | 4 +++-
> >> > include/net/bond_options.h | 4 +++-
> >> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/net/bonding/bond_options.c
> >> b/drivers/net/bonding/bond_options.c
> >> >index 0cf25de6f46d..6d2f44b3528d 100644
> >> >--- a/drivers/net/bonding/bond_options.c
> >> >+++ b/drivers/net/bonding/bond_options.c
> >> >@@ -387,7 +387,7 @@ static const struct bond_option
> >> bond_opts[BOND_OPT_LAST] = {
> >> > .id = BOND_OPT_SLAVES,
> >> > .name = "slaves",
> >> > .desc = "Slave membership management",
> >> >- .flags = BOND_OPTFLAG_RAWVAL,
> >> >+ .flags = BOND_OPTFLAG_RAWVAL | BOND_OPTFLAG_IFUP,
> >> > .set = bond_option_slaves_set
> >> > },
> >> > [BOND_OPT_TLB_DYNAMIC_LB] = {
> >> >@@ -583,6 +583,8 @@ static int bond_opt_check_deps(struct bonding
> >> *bond,
> >> > return -ENOTEMPTY;
> >> > if ((opt->flags & BOND_OPTFLAG_IFDOWN) && (bond->dev->flags &
> >> IFF_UP))
> >> > return -EBUSY;
> >> >+ if ((opt->flags & BOND_OPTFLAG_IFUP) && !(bond->dev->flags &
> >> IFF_UP))
> >> >+ return -EPERM;
> >> >
> >> > return 0;
> >> > }
> >> >diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> >> >index 9d382f2f0bc5..742f5cc81adf 100644
> >> >--- a/include/net/bond_options.h
> >> >+++ b/include/net/bond_options.h
> >> >@@ -15,11 +15,13 @@
> >> > * BOND_OPTFLAG_NOSLAVES - check if the bond device is empty
> before
> >> setting
> >> > * BOND_OPTFLAG_IFDOWN - check if the bond device is down before
> >> setting
> >> > * BOND_OPTFLAG_RAWVAL - the option parses the value itself
> >> >+ * BOND_OPTFLAG_IFUP - check if the bond device is up before setting
> >> > */
> >> > enum {
> >> > BOND_OPTFLAG_NOSLAVES = BIT(0),
> >> > BOND_OPTFLAG_IFDOWN = BIT(1),
> >> >- BOND_OPTFLAG_RAWVAL = BIT(2)
> >> >+ BOND_OPTFLAG_RAWVAL = BIT(2),
> >> >+ BOND_OPTFLAG_IFUP = BIT(3)
> >> > };
> >> >
> >> > /* Value type flags:
> >> >--
> >> >2.27.0
> >> >
>
> ---
> -Jay Vosburgh, jay.vosburgh@...onical.com
Powered by blists - more mailing lists