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

[Eric Dumazet <eric.dumazet@gmail.com>]

On 6/22/21 8:16 PM, Jay Vosburgh wrote:
> zhudi <zhudi21@huawei.com> wrote:
> 
>> From: Di Zhu <zhudi21@huawei.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).