* [PATCH net-next] bonding: permit enslaving interfaces without set_mac support
@ 2014-07-15 11:26 Veaceslav Falico
2014-07-15 11:45 ` Varka Bhadram
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Veaceslav Falico @ 2014-07-15 11:26 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
Currently we exit if the slave isn't the first slave, doesn't support mac
address setting and fail_over_mac isn't FOM_ACTIVE. It's wrong because we
only require ndo_set_mac_address in case bonding is in active-backup mode
and FOM isn't FOM_ACTIVE.
To fix this - only exit with an error if we're in a/b mode and have
fail_over_mac != FOM_ACTIVE.
Also, maintain current behaviour on the first slave (forcibly change fom to
FOM_ACTIVE) to not break anyone's configuration.
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>
---
drivers/net/bonding/bond_main.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 09dc3ef..e0a33b5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1298,19 +1298,20 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
}
if (slave_ops->ndo_set_mac_address == NULL) {
- if (!bond_has_slaves(bond)) {
- pr_warn("%s: Warning: The first slave device specified does not support setting the MAC address\n",
- bond_dev->name);
- if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) {
+ pr_warn("%s: Warning: The slave device specified does not support setting the MAC address\n",
+ bond_dev->name);
+ if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
+ bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
+ if (!bond_has_slaves(bond)) {
bond->params.fail_over_mac = BOND_FOM_ACTIVE;
pr_warn("%s: Setting fail_over_mac to active for active-backup mode\n",
bond_dev->name);
+ } else {
+ pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n",
+ bond_dev->name);
+ res = -EOPNOTSUPP;
+ goto err_undo_flags;
}
- } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
- pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n",
- bond_dev->name);
- res = -EOPNOTSUPP;
- goto err_undo_flags;
}
}
--
1.8.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] bonding: permit enslaving interfaces without set_mac support 2014-07-15 11:26 [PATCH net-next] bonding: permit enslaving interfaces without set_mac support Veaceslav Falico @ 2014-07-15 11:45 ` Varka Bhadram 2014-07-15 11:59 ` Veaceslav Falico 2014-07-16 5:55 ` David Miller 2014-07-17 19:21 ` Jay Vosburgh 2 siblings, 1 reply; 6+ messages in thread From: Varka Bhadram @ 2014-07-15 11:45 UTC (permalink / raw) To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek On 07/15/2014 04:56 PM, Veaceslav Falico wrote: > Currently we exit if the slave isn't the first slave, doesn't support mac > address setting and fail_over_mac isn't FOM_ACTIVE. It's wrong because we > only require ndo_set_mac_address in case bonding is in active-backup mode > and FOM isn't FOM_ACTIVE. > > To fix this - only exit with an error if we're in a/b mode and have > fail_over_mac != FOM_ACTIVE. > > Also, maintain current behaviour on the first slave (forcibly change fom to > FOM_ACTIVE) to not break anyone's configuration. > > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > drivers/net/bonding/bond_main.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 09dc3ef..e0a33b5 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1298,19 +1298,20 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > } > > if (slave_ops->ndo_set_mac_address == NULL) { > - if (!bond_has_slaves(bond)) { > - pr_warn("%s: Warning: The first slave device specified does not support setting the MAC address\n", > - bond_dev->name); > - if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { > + pr_warn("%s: Warning: The slave device specified does not support setting the MAC address\n", > + bond_dev->name); netdev_warn() instead of pr_warn()... We are having net_device object? > + if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP && > + bond->params.fail_over_mac != BOND_FOM_ACTIVE) { > + if (!bond_has_slaves(bond)) { > bond->params.fail_over_mac = BOND_FOM_ACTIVE; > pr_warn("%s: Setting fail_over_mac to active for active-backup mode\n", > bond_dev->name); > + } else { > + pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", > + bond_dev->name); netdev_err()...? > + res = -EOPNOTSUPP; > + goto err_undo_flags; > } > - } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) { > - pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", > - bond_dev->name); > - res = -EOPNOTSUPP; > - goto err_undo_flags; > } > } > -- Regards, Varka Bhadram. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] bonding: permit enslaving interfaces without set_mac support 2014-07-15 11:45 ` Varka Bhadram @ 2014-07-15 11:59 ` Veaceslav Falico 0 siblings, 0 replies; 6+ messages in thread From: Veaceslav Falico @ 2014-07-15 11:59 UTC (permalink / raw) To: Varka Bhadram; +Cc: netdev, Jay Vosburgh, Andy Gospodarek On Tue, Jul 15, 2014 at 05:15:34PM +0530, Varka Bhadram wrote: >On 07/15/2014 04:56 PM, Veaceslav Falico wrote: ...snip... >>- pr_warn("%s: Warning: The first slave device specified does not support setting the MAC address\n", >>- bond_dev->name); >>- if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { >>+ pr_warn("%s: Warning: The slave device specified does not support setting the MAC address\n", >>+ bond_dev->name); > >netdev_warn() instead of pr_warn()... We are having net_device object? It's done in consistency with the whole function - there are pr_* everywhere there, and netdev_x here and there would only mess things up. I'll send a follow-up patch that transforms all pr_* into netdev_*. Thank you! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] bonding: permit enslaving interfaces without set_mac support 2014-07-15 11:26 [PATCH net-next] bonding: permit enslaving interfaces without set_mac support Veaceslav Falico 2014-07-15 11:45 ` Varka Bhadram @ 2014-07-16 5:55 ` David Miller 2014-07-17 19:21 ` Jay Vosburgh 2 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2014-07-16 5:55 UTC (permalink / raw) To: vfalico; +Cc: netdev, j.vosburgh, andy From: Veaceslav Falico <vfalico@gmail.com> Date: Tue, 15 Jul 2014 13:26:01 +0200 > Currently we exit if the slave isn't the first slave, doesn't support mac > address setting and fail_over_mac isn't FOM_ACTIVE. It's wrong because we > only require ndo_set_mac_address in case bonding is in active-backup mode > and FOM isn't FOM_ACTIVE. > > To fix this - only exit with an error if we're in a/b mode and have > fail_over_mac != FOM_ACTIVE. > > Also, maintain current behaviour on the first slave (forcibly change fom to > FOM_ACTIVE) to not break anyone's configuration. > > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> Applied, thank you. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] bonding: permit enslaving interfaces without set_mac support 2014-07-15 11:26 [PATCH net-next] bonding: permit enslaving interfaces without set_mac support Veaceslav Falico 2014-07-15 11:45 ` Varka Bhadram 2014-07-16 5:55 ` David Miller @ 2014-07-17 19:21 ` Jay Vosburgh 2014-07-17 19:31 ` Veaceslav Falico 2 siblings, 1 reply; 6+ messages in thread From: Jay Vosburgh @ 2014-07-17 19:21 UTC (permalink / raw) To: Veaceslav Falico; +Cc: netdev, Andy Gospodarek Veaceslav Falico <vfalico@gmail.com> wrote: >Currently we exit if the slave isn't the first slave, doesn't support mac >address setting and fail_over_mac isn't FOM_ACTIVE. It's wrong because we >only require ndo_set_mac_address in case bonding is in active-backup mode >and FOM isn't FOM_ACTIVE. > >To fix this - only exit with an error if we're in a/b mode and have >fail_over_mac != FOM_ACTIVE. > >Also, maintain current behaviour on the first slave (forcibly change fom to >FOM_ACTIVE) to not break anyone's configuration. I'm just catching up on this, so I apologize for being late to the party here, but the main point of that test was to prohibit slaves that cannot change their MAC from joining a bond whose mode requires changing the MAC (which is all of them except for active-backup). It looks like the new code path will still fail when bond_enslave() later on attempts to change the MAC, as long as fail_over_mac is not set. If f_o_m is set, the enslave will succeed, after bypassing the dev_set_mac_address call. That in turn would seem to allow creating a bond of any mode, setting fail_over_mac, then filling it with, e.g., ipoib devices. That bond won't function correctly in the modes other than active-backup. Am I missing something here? -J >CC: Jay Vosburgh <j.vosburgh@gmail.com> >CC: Andy Gospodarek <andy@greyhouse.net> >Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >--- > drivers/net/bonding/bond_main.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 09dc3ef..e0a33b5 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1298,19 +1298,20 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > } > > if (slave_ops->ndo_set_mac_address == NULL) { >- if (!bond_has_slaves(bond)) { >- pr_warn("%s: Warning: The first slave device specified does not support setting the MAC address\n", >- bond_dev->name); >- if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { >+ pr_warn("%s: Warning: The slave device specified does not support setting the MAC address\n", >+ bond_dev->name); >+ if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP && >+ bond->params.fail_over_mac != BOND_FOM_ACTIVE) { >+ if (!bond_has_slaves(bond)) { > bond->params.fail_over_mac = BOND_FOM_ACTIVE; > pr_warn("%s: Setting fail_over_mac to active for active-backup mode\n", > bond_dev->name); >+ } else { >+ pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", >+ bond_dev->name); >+ res = -EOPNOTSUPP; >+ goto err_undo_flags; > } >- } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) { >- pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", >- bond_dev->name); >- res = -EOPNOTSUPP; >- goto err_undo_flags; > } > } > >-- >1.8.4 > --- -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] bonding: permit enslaving interfaces without set_mac support 2014-07-17 19:21 ` Jay Vosburgh @ 2014-07-17 19:31 ` Veaceslav Falico 0 siblings, 0 replies; 6+ messages in thread From: Veaceslav Falico @ 2014-07-17 19:31 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, Andy Gospodarek On Thu, Jul 17, 2014 at 12:21:31PM -0700, Jay Vosburgh wrote: >Veaceslav Falico <vfalico@gmail.com> wrote: > >>Currently we exit if the slave isn't the first slave, doesn't support mac >>address setting and fail_over_mac isn't FOM_ACTIVE. It's wrong because we >>only require ndo_set_mac_address in case bonding is in active-backup mode >>and FOM isn't FOM_ACTIVE. >> >>To fix this - only exit with an error if we're in a/b mode and have >>fail_over_mac != FOM_ACTIVE. >> >>Also, maintain current behaviour on the first slave (forcibly change fom to >>FOM_ACTIVE) to not break anyone's configuration. > > I'm just catching up on this, so I apologize for being late to >the party here, but the main point of that test was to prohibit slaves >that cannot change their MAC from joining a bond whose mode requires >changing the MAC (which is all of them except for active-backup). > > It looks like the new code path will still fail when >bond_enslave() later on attempts to change the MAC, as long as >fail_over_mac is not set. If f_o_m is set, the enslave will succeed, >after bypassing the dev_set_mac_address call. That in turn would seem >to allow creating a bond of any mode, setting fail_over_mac, then >filling it with, e.g., ipoib devices. That bond won't function >correctly in the modes other than active-backup. Hm, good catch, it wasn't covered in my testing I guess. I don't have the time/code at hand right now, so can't re-test it. I'll verify it tomorrow and, if it's true (most probably), will send a follow-up. Thank you! > > Am I missing something here? > > -J > >>CC: Jay Vosburgh <j.vosburgh@gmail.com> >>CC: Andy Gospodarek <andy@greyhouse.net> >>Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >>--- >> drivers/net/bonding/bond_main.c | 19 ++++++++++--------- >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >>index 09dc3ef..e0a33b5 100644 >>--- a/drivers/net/bonding/bond_main.c >>+++ b/drivers/net/bonding/bond_main.c >>@@ -1298,19 +1298,20 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) >> } >> >> if (slave_ops->ndo_set_mac_address == NULL) { >>- if (!bond_has_slaves(bond)) { >>- pr_warn("%s: Warning: The first slave device specified does not support setting the MAC address\n", >>- bond_dev->name); >>- if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { >>+ pr_warn("%s: Warning: The slave device specified does not support setting the MAC address\n", >>+ bond_dev->name); >>+ if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP && >>+ bond->params.fail_over_mac != BOND_FOM_ACTIVE) { >>+ if (!bond_has_slaves(bond)) { >> bond->params.fail_over_mac = BOND_FOM_ACTIVE; >> pr_warn("%s: Setting fail_over_mac to active for active-backup mode\n", >> bond_dev->name); >>+ } else { >>+ pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", >>+ bond_dev->name); >>+ res = -EOPNOTSUPP; >>+ goto err_undo_flags; >> } >>- } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) { >>- pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n", >>- bond_dev->name); >>- res = -EOPNOTSUPP; >>- goto err_undo_flags; >> } >> } >> >>-- >>1.8.4 >> > >--- > -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-17 19:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-15 11:26 [PATCH net-next] bonding: permit enslaving interfaces without set_mac support Veaceslav Falico 2014-07-15 11:45 ` Varka Bhadram 2014-07-15 11:59 ` Veaceslav Falico 2014-07-16 5:55 ` David Miller 2014-07-17 19:21 ` Jay Vosburgh 2014-07-17 19:31 ` Veaceslav Falico
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).