netdev - Re: [PATCH] bonding: Avoid adding slave devices to inactive bonding

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-next>] [day] [month] [year] [list]

Message-ID: <a457335cb9a04023808f5b34cd8c1d30@huawei.com>
Date:   Wed, 28 Jul 2021 06:51:55 +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.

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

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