Re: [PATCH net] net/bonding: Enforce active-backup policy for IPoIB bonds

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Andy Gospodarek <gospo@cumulusnetworks.com> wrote:

>On Wed, Jul 20, 2016 at 05:44:20PM +0300, Saeed Mahameed wrote:
>> From: Mark Bloch <markb@mellanox.com>
>> 
>> When using an IPoIB bond currently only active-backup mode is a valid
>> use case and this commit strengthens it.
>> 
>> Since commit 2ab82852a270 ("net/bonding: Enable bonding to enslave
>> netdevices not supporting set_mac_address()") was introduced till
>> 4.7-rc1, IPoIB didn't support the set_mac_address ndo, and hence
>> the fail over mac policy always applied to IPoIB bonds.
>> 
>> With the introduction of commit 492a7e67ff83 ("IB/IPoIB: Allow setting
>> the device address"), that doesn't hold and practically IPoIB bonds are
>> broken as of that. To fix it, lets go to fail over mac if the device
>> doesn't support the ndo OR this is IPoIB device.
>> 
>> As a by-product, this commit also prevents a stack corruption which
>> occurred when trying to copy 20 bytes (IPoIB) device address
>> to a sockaddr struct that has only 16 bytes of storage.
>> 
>> Signed-off-by: Mark Bloch <markb@mellanox.com>
>> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index a2afa3b..ccd4003 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1422,7 +1422,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (slave_ops->ndo_set_mac_address == NULL) {
>> +	if (slave_dev->type == ARPHRD_INFINIBAND &&
>> +	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +		netdev_warn(bond_dev, "Type (%d) supports only active-backup mode\n",
>> +			    slave_dev->type);
>
>Seems like we should propagate the failure back to the caller.
>Something like this?
>
>		return -EOPNOTSUPP;

	Agreed, although I think it needs to be

	res = -EOPNOTSUPP;
	goto err_undo_flags;

	The code has to undo the the ARPHRD_INFINIBAND setting in
bond_dev->type and other stuff done by bond_setup_by_slave.  If we don't
switch the fields back to the ARPHRD_ETHER related values, then we could
have an invalid dereference on the header_ops pointer if something
unloads the IB module.

	-J

>> +		goto err_undo_flags;
>> +	}
>> +
>> +	if (!slave_ops->ndo_set_mac_address ||
>> +	    slave_dev->type == ARPHRD_INFINIBAND) {
>>  		netdev_warn(bond_dev, "The slave device specified does not support setting the MAC address\n");
>>  		if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
>>  		    bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
>
>The rest of the patch seems logical, so I'm fine with it.

---
	-Jay Vosburgh, jay.vosburgh@canonical.com