* [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).