| 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: <5226.1627502270@famine>
Date: Wed, 28 Jul 2021 12:57:50 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: "zhudi (J)" <zhudi21@...wei.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 (J) <zhudi21@...wei.com> wrote:
>> 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.
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.
-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