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

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

[zhudi]

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.

Signed-off-by: Di Zhu <zhudi21@huawei.com>
---
 drivers/net/bonding/bond_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c5a646d06102..16840c9bc00d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1601,6 +1601,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	int link_reporting;
 	int res = 0, i;
 
+	if (slave_dev->flags & IFF_MASTER) {
+		netdev_err(bond_dev,
+			   "Error: Device with IFF_MASTER cannot be enslaved\n");
+		return -EPERM;
+	}
+
 	if (!bond->params.use_carrier &&
 	    slave_dev->ethtool_ops->get_link == NULL &&
 	    slave_ops->ndo_do_ioctl == NULL) {
-- 
2.23.0

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue, 22 Jun 2021 11:09:29 +0800 you 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
> 
> [...]

Here is the summary with links:
  - bonding: avoid adding slave device with IFF_MASTER flag
    https://git.kernel.org/netdev/net/c/3c9ef511b9fa

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Eric Dumazet]

On 6/22/21 5:09 AM, zhudi 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.
> 
> Signed-off-by: Di Zhu <zhudi21@huawei.com>
> ---
>  drivers/net/bonding/bond_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c5a646d06102..16840c9bc00d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1601,6 +1601,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	int link_reporting;
>  	int res = 0, i;
>  
> +	if (slave_dev->flags & IFF_MASTER) {

Missing NL_SET_ERR_MSG( ?

> +		netdev_err(bond_dev,
> +			   "Error: Device with IFF_MASTER cannot be enslaved\n");
> +		return -EPERM;
> +	}
> +
>  	if (!bond->params.use_carrier &&
>  	    slave_dev->ethtool_ops->get_link == NULL &&
>  	    slave_ops->ndo_do_ioctl == NULL) {
> 

This is strange, can you tell us why we keep the following lines after your patch ?

	if (bond_dev == slave_dev) {
		NL_SET_ERR_MSG(extack, "Cannot enslave bond to itself.");
		netdev_err(bond_dev, "cannot enslave bond to itself.\n");
		return -EPERM;
	}

I was under the impression that a stack of bonding devices was allowed.

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

[Jay Vosburgh]

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.

	-J

>Signed-off-by: Di Zhu <zhudi21@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index c5a646d06102..16840c9bc00d 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1601,6 +1601,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 	int link_reporting;
> 	int res = 0, i;
> 
>+	if (slave_dev->flags & IFF_MASTER) {
>+		netdev_err(bond_dev,
>+			   "Error: Device with IFF_MASTER cannot be enslaved\n");
>+		return -EPERM;
>+	}
>+
> 	if (!bond->params.use_carrier &&
> 	    slave_dev->ethtool_ops->get_link == NULL &&
> 	    slave_ops->ndo_do_ioctl == NULL) {
>-- 
>2.23.0
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

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

[Eric Dumazet]

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

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

[zhudi (J)]

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

	Thank you for your advice, as Eric dumazet described: since there is a usage scenario
about nesting bonding, we should not break it.

> 
> >Signed-off-by: Di Zhu <zhudi21@huawei.com>
> >---
> > drivers/net/bonding/bond_main.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> >index c5a646d06102..16840c9bc00d 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1601,6 +1601,12 @@ int bond_enslave(struct net_device *bond_dev,
> struct net_device *slave_dev,
> > 	int link_reporting;
> > 	int res = 0, i;
> >
> >+	if (slave_dev->flags & IFF_MASTER) {
> >+		netdev_err(bond_dev,
> >+			   "Error: Device with IFF_MASTER cannot be
> enslaved\n");
> >+		return -EPERM;
> >+	}
> >+
> > 	if (!bond->params.use_carrier &&
> > 	    slave_dev->ethtool_ops->get_link == NULL &&
> > 	    slave_ops->ndo_do_ioctl == NULL) {
> >--
> >2.23.0
> >
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

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

[zhudi (J)]

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

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.