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

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

[Saeed Mahameed]

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);
+		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) {
-- 
2.8.0

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

[Andy Gospodarek]

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;
> +		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.

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

[Jay Vosburgh]

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

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

[Andy Gospodarek]

On Wed, Jul 20, 2016 at 10:13:47AM -0700, Jay Vosburgh wrote:
> 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;
> 

Good catch, Jay.  That was _actually_ what I meant to put as we do want
to unroll everything, but apparently my paste buffer was wrong!

> 	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