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: <21906.1440733165@nyx>
Date:	Thu, 27 Aug 2015 20:39:25 -0700
From:	Jay Vosburgh <jay.vosburgh@...onical.com>
To:	Nikolay Aleksandrov <nikolay@...ulusnetworks.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

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

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

	-J

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