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: <55A6C3CB.9070605@cumulusnetworks.com>
Date:	Wed, 15 Jul 2015 22:34:19 +0200
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	Jay Vosburgh <jay.vosburgh@...onical.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>
CC:	netdev@...r.kernel.org, monis@...taire.com,
	gospo@...ulusnetworks.com, vfalico@...il.com, davem@...emloft.net
Subject: Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

On 07/15/2015 10:32 PM, Jay Vosburgh 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
> 
> 
>> 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
> 

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.

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