> On Jul 15, 2015, at 1:32 PM, Jay Vosburgh <jay.vosbu...@canonical.com> wrote:
>
> Nikolay Aleksandrov <ra...@blackwall.org> wrote:
>
>> From: Nikolay Aleksandrov <niko...@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 <niko...@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.vosbu...@canonical.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
