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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 28 Aug 2015 10:38:59 -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 Aug 27, 2015, at 8:39 PM, Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
> 
> Nikolay Aleksandrov <nikolay@...ulusnetworks.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@...onical.com

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

Thanks,
 Nik--
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