Andy Gospodarek <go...@cumulusnetworks.com> wrote: >On Wed, Jul 20, 2016 at 05:44:20PM +0300, Saeed Mahameed wrote: >> From: Mark Bloch <ma...@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 <ma...@mellanox.com> >> Signed-off-by: Or Gerlitz <ogerl...@mellanox.com> >> Signed-off-by: Saeed Mahameed <sae...@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.vosbu...@canonical.com