[PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

If a bonding device enslaves devices != arphrd_ether it'll change types
and if later these devices are released, it can enslave an arphrd_ether
device and switch back calling ether_setup() which resets dev->flags to
IFF_BROADCAST|IFF_MULTICAST and clears IFF_MASTER which then could lead
to many different bugs. This bug seems to have been there since the
introduction of ether_setup() in bond_enslave().

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 317a49480475..8ba119896e55 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1368,6 +1368,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 				bond_setup_by_slave(bond_dev, slave_dev);
 			else {
 				ether_setup(bond_dev);
+				bond_dev->flags |= IFF_MASTER;
 				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 			}
 
-- 
1.9.3

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Jay Vosburgh]

Nikolay Aleksandrov <razor@blackwall.org> wrote:

>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
>If a bonding device enslaves devices != arphrd_ether it'll change types
>and if later these devices are released, it can enslave an arphrd_ether
>device and switch back calling ether_setup() which resets dev->flags to
>IFF_BROADCAST|IFF_MULTICAST and clears IFF_MASTER which then could lead
>to many different bugs. This bug seems to have been there since the
>introduction of ether_setup() in bond_enslave().

	I thought the hack-around for the non-ethernet device problem
was that, for non-ARPHRD_ETHER devices, the bonding master device is
automatically destroyed when the last slave is released.

	This (well, this sort of thing) originally came up with IPoIB
devices needing different ops that would disappear if the ipoib module
was unloaded, so the bond master was deliberately unregistered when the
last slave was released.

	Looking at the code, at first glance this appears to still be
the case: bond_slave_netdev_event calls bond_release_and_destroy for !=
ARPHRD_ETHER, which in turn should unregister the bond itself if there
are no slaves left.  Is this no longer working as intended?

	-J


>Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>---
> drivers/net/bonding/bond_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 317a49480475..8ba119896e55 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1368,6 +1368,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 				bond_setup_by_slave(bond_dev, slave_dev);
> 			else {
> 				ether_setup(bond_dev);
>+				bond_dev->flags |= IFF_MASTER;
> 				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> 			}
> 
>-- 
>1.9.3
>

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

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Nikolay Aleksandrov]

On 07/15/2015 10:32 PM, Jay Vosburgh wrote:
> Nikolay Aleksandrov <razor@blackwall.org> wrote:
> 
>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>
>> If a bonding device enslaves devices != arphrd_ether it'll change types
>> and if later these devices are released, it can enslave an arphrd_ether
>> device and switch back calling ether_setup() which resets dev->flags to
>> IFF_BROADCAST|IFF_MULTICAST and clears IFF_MASTER which then could lead
>> to many different bugs. This bug seems to have been there since the
>> introduction of ether_setup() in bond_enslave().
> 
> 	I thought the hack-around for the non-ethernet device problem
> was that, for non-ARPHRD_ETHER devices, the bonding master device is
> automatically destroyed when the last slave is released.
> 
> 	This (well, this sort of thing) originally came up with IPoIB
> devices needing different ops that would disappear if the ipoib module
> was unloaded, so the bond master was deliberately unregistered when the
> last slave was released.
> 
> 	Looking at the code, at first glance this appears to still be
> the case: bond_slave_netdev_event calls bond_release_and_destroy for !=
> ARPHRD_ETHER, which in turn should unregister the bond itself if there
> are no slaves left.  Is this no longer working as intended?
> 
> 	-J
> 
> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>> ---
>> drivers/net/bonding/bond_main.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 317a49480475..8ba119896e55 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1368,6 +1368,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 				bond_setup_by_slave(bond_dev, slave_dev);
>> 			else {
>> 				ether_setup(bond_dev);
>> +				bond_dev->flags |= IFF_MASTER;
>> 				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> 			}
>>
>> -- 
>> 1.9.3
>>
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> 

Ah yes, the bug is actually that the bond doesn't switch back its type
on failure after the bond_setup_by_slave().
I actually had prepared that fix next. :-)
But you're right, this doesn't need to be fixed if the enslave failure
path is updated to switch back the type on fail.

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Nikolay Aleksandrov]

On 07/15/2015 10:09 PM, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 
> If a bonding device enslaves devices != arphrd_ether it'll change types
> and if later these devices are released, it can enslave an arphrd_ether
> device and switch back calling ether_setup() which resets dev->flags to
> IFF_BROADCAST|IFF_MULTICAST and clears IFF_MASTER which then could lead
> to many different bugs. This bug seems to have been there since the
> introduction of ether_setup() in bond_enslave().
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
> ---
>  drivers/net/bonding/bond_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 317a49480475..8ba119896e55 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1368,6 +1368,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  				bond_setup_by_slave(bond_dev, slave_dev);
>  			else {
>  				ether_setup(bond_dev);
> +				bond_dev->flags |= IFF_MASTER;
>  				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>  			}
>  
> 

Dave please ignore this patch, I'll send a better fix.

Thanks,
 Nik

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Wed, 15 Jul 2015 22:55:55 +0200

> Dave please ignore this patch, I'll send a better fix.

OK

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Nikolay Aleksandrov]

> On Jul 15, 2015, at 1:32 PM, Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> 
> Nikolay Aleksandrov <razor@blackwall.org> wrote:
> 
>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> 
>> If a bonding device enslaves devices != arphrd_ether it'll change types
>> and if later these devices are released, it can enslave an arphrd_ether
>> device and switch back calling ether_setup() which resets dev->flags to
>> IFF_BROADCAST|IFF_MULTICAST and clears IFF_MASTER which then could lead
>> to many different bugs. This bug seems to have been there since the
>> introduction of ether_setup() in bond_enslave().
> 
> 	I thought the hack-around for the non-ethernet device problem
> was that, for non-ARPHRD_ETHER devices, the bonding master device is
> automatically destroyed when the last slave is released.
> 
> 	This (well, this sort of thing) originally came up with IPoIB
> devices needing different ops that would disappear if the ipoib module
> was unloaded, so the bond master was deliberately unregistered when the
> last slave was released.
> 
> 	Looking at the code, at first glance this appears to still be
> the case: bond_slave_netdev_event calls bond_release_and_destroy for !=
> ARPHRD_ETHER, which in turn should unregister the bond itself if there
> are no slaves left.  Is this no longer working as intended?
> 
> 	-J
> 
Restarting this thread because there’s actually a bug here, what you described with
the bonding destruction is true when the slaves are all destroyed but it isn’t true if they’re
just released, if you take a look at bond_slave_netdev_event() the bond destruction happens
only on NETDEV_UNREGISTER and I just hit this bug by enslaving a non-ARPHRD_ETHER
device, releasing it and enslaving a ARPHRD_ETHER device so ether_setup() path in bond_enslave
is hit and IFF_MASTER gets dropped:
17: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/fddi 9a:33:c5:30:ff:a6 brd ff:ff:ff:ff:ff:ff
(release non-ARPHRD_ETHER slave)
(enslave ARPHRD_ETHER device)
17: bond0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 08:00:27:3c:13:57 brd ff:ff:ff:ff:ff:ff

Notice the master flag is gone and of course on unload we get:
[57981.545547] ------------[ cut here ]------------
[57981.545567] WARNING: CPU: 0 PID: 13792 at fs/proc/generic.c:575 remove_proc_entry+0x17e/0x190()
[57981.545572] remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 'bond0'
[57981.545576] Modules linked in: bonding(-) bridge(OE) stp llc rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache macvlan netconsole ppdev joydev serio_raw parport_pc parport i2c_piix4 video acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace sunrpc virtio_net pcnet32 mii virtio_pci virtio_ring virtio e1000 ata_generic pata_acpi [last unloaded: bonding]
[57981.545614] CPU: 0 PID: 13792 Comm: rmmod Tainted: G           OE   4.2.0-rc7+ #56
[57981.545618] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[57981.545623]  0000000000000000 00000000b15c0564 ffff88001ec43ca8 ffffffff8183f105
[57981.545629]  0000000000000000 ffff88001ec43d00 ffff88001ec43ce8 ffffffff810a9496
[57981.545635]  0000000000000000 ffff88002ef6a800 ffff88002ef6a838 ffff88002e24ed00
[57981.545641] Call Trace:
[57981.545649]  [<ffffffff8183f105>] dump_stack+0x4c/0x65
[57981.545656]  [<ffffffff810a9496>] warn_slowpath_common+0x86/0xc0
[57981.545662]  [<ffffffff810a9525>] warn_slowpath_fmt+0x55/0x70
[57981.545669]  [<ffffffff812dbcde>] remove_proc_entry+0x17e/0x190
[57981.545675]  [<ffffffff812e79d5>] ? kernfs_remove_by_name_ns+0x55/0xa0
[57981.545687]  [<ffffffffa032522e>] bond_destroy_proc_dir+0x2e/0x3e [bonding]
[57981.545694]  [<ffffffffa03126fe>] bond_net_exit+0x13e/0x200 [bonding]
[57981.545700]  [<ffffffffa03125c5>] ? bond_net_exit+0x5/0x200 [bonding]
[57981.545707]  [<ffffffff816bd7c8>] ops_exit_list.isra.4+0x38/0x60
[57981.545713]  [<ffffffff816bdd08>] unregister_pernet_operations+0x78/0xd0
[57981.545718]  [<ffffffff816bdd87>] unregister_pernet_subsys+0x27/0x40
[57981.545726]  [<ffffffffa032546b>] bonding_exit+0x26/0xbbb [bonding]
[57981.545732]  [<ffffffff8114a9f7>] SyS_delete_module+0x1b7/0x210
[57981.545739]  [<ffffffff81003017>] ? trace_hardirqs_on_thunk+0x17/0x19
[57981.545745]  [<ffffffff8184936e>] entry_SYSCALL_64_fastpath+0x12/0x76
[57981.545749] ---[ end trace 46a4798bb28254d0 ]—

We need to convert it back to ARPHRD_ETHER if releasing the last slave, because
we can’t destroy it (in some paths bond->dev is used after bond_release()).
Basically we should make the case that if the bonding doesn’t have any slaves then it’s
always an ARPHRD_ETHER device.

Thoughts ?

> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>> ---
>> drivers/net/bonding/bond_main.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 317a49480475..8ba119896e55 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1368,6 +1368,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 				bond_setup_by_slave(bond_dev, slave_dev);
>> 			else {
>> 				ether_setup(bond_dev);
>> +				bond_dev->flags |= IFF_MASTER;
>> 				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> 			}
>> 
>> -- 
>> 1.9.3
>> 
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Jay Vosburgh]

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
[...]
>Restarting this thread because there’s actually a bug here, what you described with
>the bonding destruction is true when the slaves are all destroyed but it isn’t true if they’re
>just released, if you take a look at bond_slave_netdev_event() the bond destruction happens
>only on NETDEV_UNREGISTER and I just hit this bug by enslaving a non-ARPHRD_ETHER
>device, releasing it and enslaving a ARPHRD_ETHER device so ether_setup() path in bond_enslave
>is hit and IFF_MASTER gets dropped:
>17: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/fddi 9a:33:c5:30:ff:a6 brd ff:ff:ff:ff:ff:ff
>(release non-ARPHRD_ETHER slave)
>(enslave ARPHRD_ETHER device)
>17: bond0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/ether 08:00:27:3c:13:57 brd ff:ff:ff:ff:ff:ff
>
>Notice the master flag is gone and of course on unload we get:
>[57981.545547] ------------[ cut here ]------------
>[57981.545567] WARNING: CPU: 0 PID: 13792 at fs/proc/generic.c:575 remove_proc_entry+0x17e/0x190()
>[57981.545572] remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 'bond0'
[...]
>We need to convert it back to ARPHRD_ETHER if releasing the last slave, because
>we can’t destroy it (in some paths bond->dev is used after bond_release()).
>Basically we should make the case that if the bonding doesn’t have any slaves then it’s
>always an ARPHRD_ETHER device.
>
>Thoughts ?

	I agree that it would be cleaner for bond_dev->type to switch
back on release of last slave.  The options code (caller of
bond_option_slaves_set) and bond_uninit() both reference the bond or dev
after calling bond_release(), and would need changing if any release
could destroy the bond itself.

	However, for the type change, there's the potentially tricky
case of a nested non-ARPHRD_ETHER bond, e.g., bond0 -> bond1 -> ib0.
This isn't a typical use case that I'm aware of, but I believe it's
supported by the code.

	If ib0, the last slave, is released, bond1 will want to change
to ARPHRD_ETHER, but bond0 is ARPHRD_INFINIBAND.  I suspect bonding will
have to notice the NETDEV_PRE_TYPE_CHANGE and _POST_ notifiers and take
appropriate action (i.e., cascade the type change upwards).

	There might be similar issues with other devices stacked on top
of the IB -> Ether type-changing bond; I'm not sure how many of those
there may be, though, since many things won't stack over IB devices (or
an IB-flavor bond).

	If the type change works, then I don't think we would still need
the "release and destroy" logic.

	-J

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

Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Nikolay Aleksandrov]

> On Aug 27, 2015, at 8:39 PM, Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> 
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> [...]
>> Restarting this thread because there’s actually a bug here, what you described with
>> the bonding destruction is true when the slaves are all destroyed but it isn’t true if they’re
>> just released, if you take a look at bond_slave_netdev_event() the bond destruction happens
>> only on NETDEV_UNREGISTER and I just hit this bug by enslaving a non-ARPHRD_ETHER
>> device, releasing it and enslaving a ARPHRD_ETHER device so ether_setup() path in bond_enslave
>> is hit and IFF_MASTER gets dropped:
>> 17: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>>   link/fddi 9a:33:c5:30:ff:a6 brd ff:ff:ff:ff:ff:ff
>> (release non-ARPHRD_ETHER slave)
>> (enslave ARPHRD_ETHER device)
>> 17: bond0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>>   link/ether 08:00:27:3c:13:57 brd ff:ff:ff:ff:ff:ff
>> 
>> Notice the master flag is gone and of course on unload we get:
>> [57981.545547] ------------[ cut here ]------------
>> [57981.545567] WARNING: CPU: 0 PID: 13792 at fs/proc/generic.c:575 remove_proc_entry+0x17e/0x190()
>> [57981.545572] remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 'bond0'
> [...]
>> We need to convert it back to ARPHRD_ETHER if releasing the last slave, because
>> we can’t destroy it (in some paths bond->dev is used after bond_release()).
>> Basically we should make the case that if the bonding doesn’t have any slaves then it’s
>> always an ARPHRD_ETHER device.
>> 
>> Thoughts ?
> 
> 	I agree that it would be cleaner for bond_dev->type to switch
> back on release of last slave.  The options code (caller of
> bond_option_slaves_set) and bond_uninit() both reference the bond or dev
> after calling bond_release(), and would need changing if any release
> could destroy the bond itself.
> 
> 	However, for the type change, there's the potentially tricky
> case of a nested non-ARPHRD_ETHER bond, e.g., bond0 -> bond1 -> ib0.
> This isn't a typical use case that I'm aware of, but I believe it's
> supported by the code.
> 
> 	If ib0, the last slave, is released, bond1 will want to change
> to ARPHRD_ETHER, but bond0 is ARPHRD_INFINIBAND.  I suspect bonding will
> have to notice the NETDEV_PRE_TYPE_CHANGE and _POST_ notifiers and take
> appropriate action (i.e., cascade the type change upwards).
> 
> 	There might be similar issues with other devices stacked on top
> of the IB -> Ether type-changing bond; I'm not sure how many of those
> there may be, though, since many things won't stack over IB devices (or
> an IB-flavor bond).
> 
Ugh right, this would be a problem. I’ll see if it can be handled well.

> 	If the type change works, then I don't think we would still need
> the "release and destroy" logic.
> 
Right, that was my intention.

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

I’ll look into this some more and if it works out I’ll post the patch.

Thanks,
 Nik