netdev - Re: [PATCH] bonding: avoid adding slave device with IFF

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

Date:   Wed, 23 Jun 2021 02:02:14 +0000
From:   "zhudi (J)" <zhudi21@...wei.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        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 device with IFF_MASTER flag

> 
> On 6/22/21 8:16 PM, Jay Vosburgh wrote:
> > zhudi <zhudi21@...wei.com> wrote:
> >
> >> From: Di Zhu <zhudi21@...wei.com>
> >>
> >> The following steps will definitely cause the kernel to crash:
> >> 	ip link add vrf1 type vrf table 1
> >> 	modprobe bonding.ko max_bonds=1
> >> 	echo "+vrf1" >/sys/class/net/bond0/bonding/slaves
> >> 	rmmod bonding
> >>
> >> The root cause is that: When the VRF is added to the slave device, it
> >> will fail, and some cleaning work will be done. because VRF device
> >> has IFF_MASTER flag, cleanup process  will not clear the IFF_BONDING
> flag.
> >> Then, when we unload the bonding module,
> >> unregister_netdevice_notifier() will treat the VRF device as a bond
> >> master device and treat netdev_priv() as struct bonding{} which actually is
> struct net_vrf{}.
> >>
> >> By analyzing the processing logic of bond_enslave(), it seems that it
> >> is not allowed to add the slave device with the IFF_MASTER flag, so
> >> we need to add a code check for this situation.
> >
> > 	I don't believe the statement just above is correct; nesting bonds
> > has historically been permitted, even if it is of questionable value
> > these days.  I've not tested nesting in a while, but last I recall it
> > did function.
> >
> > 	Leaving aside the question of whether it's really useful to nest
> > bonds or not, my concern with disabling this is that it will break
> > existing configurations that currently work fine.
> >
> > 	However, it should be possible to use netif_is_bonding_master
> (which
> > tests dev->flags & IFF_MASTER and dev->priv_flags & IFF_BONDING) to
> > exclude IFF_MASTER devices that are not bonds (which seem to be vrf
> > and eql), e.g.,
> >
> > 	if ((slave_dev->flags & IFF_MASTER) &&
> > 		!netif_is_bond_master(slave_dev))
> >
> > 	Or we can just go with this patch and see if anything breaks.
> >
> 
> syzbot for sure will stop finding stack overflows and other issues like that :)
> 
> I know that some people used nested bonding devices in order to implement
> complex qdisc setups.
> (eg HTB on the first level, netem on the second level).

If there is such a usage scenario,  the following code proposed by Jay Vosburgh is better:
	if ((slave_dev->flags & IFF_MASTER) && 
			!netif_is_bond_master(slave_dev))

Thank you for your advice, I will send another patch to fix it.