netdev - Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <92603003-58ED-46B1-B789-5D4765256FAC@cumulusnetworks.com>
Date:	Thu, 27 Aug 2015 19:07:41 -0700
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:	netdev@...r.kernel.org,
	Andy Gospodarek <gospo@...ulusnetworks.com>, vfalico@...il.com,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether


> On Jul 15, 2015, at 1:32 PM, Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
> 
> Nikolay Aleksandrov <razor@...ckwall.org> wrote:
> 
>> From: Nikolay Aleksandrov <nikolay@...ulusnetworks.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@...ulusnetworks.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@...onical.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html