Re: [PATCH] bonding: avoid adding slave device with IFF_MASTER flag

On 6/22/21 8:16 PM, Jay Vosburgh wrote:

quoted

zhudi [off-list ref] wrote:

quoted

From: Di Zhu <redacted>

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.

quoted

quoted

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

quoted

quoted

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

quoted

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.