[Regression] Bonding no longer support tun-interfaces

[Regression] Bonding no longer support tun-interfaces

[Jörn Engel]

This has been reported (and ignored) before:
http://lkml.iu.edu/hypermail/linux/kernel/1407.2/03790.html
https://bugzilla.kernel.org/show_bug.cgi?id=89161

Regression was introduced by:

commit 00503b6f702e (refs/bisect/bad)
Author: dingtianhong <dingtianhong@huawei.com>
Date:   Sat Jan 25 13:00:29 2014 +0800

    bonding: fail_over_mac should only affect AB mode at enslave and removal processing
    
    According to bonding.txt, the fail_over_ma should only affect active-backup mode,
    but I found that the fail_over_mac could be set to active or follow in all
    modes, this will cause new slave could not be set to bond's MAC address at
    enslave processing and restore its own MAC address at removal processing.
    
    The correct way to fix the problem is that we should not add restrictions when
    setting options, just need to modify the bond enslave and removal processing
    to check the mode in addition to fail_over_mac when setting a slave's MAC during
    enslavement. The change active slave processing already only calls the fail_over_mac
    function when in active-backup mode.
    
    Thanks for Jay's suggestion.
    
    The patch also modify the pr_warning() to pr_warn().
    
    Cc: Jay Vosburgh <fubar@us.ibm.com>
    Cc: Veaceslav Falico <vfalico@redhat.com>
    Cc: Andy Gospodarek <andy@greyhouse.net>
    Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Since I never needed bonding or tun-interfaces before, I come late to
the party.  Some 6k lines have changed in the bonding driver since the
regression got in two years ago.  So a simple revert is unlikely to lead
to happiness.

But I absolutely need that functionality and would rather run a 3.13
kernel than live with the regression.  dingtianhong, any suggestions?

Jörn

--
It is a cliché that most clichés are true, but then, like most clichés,
that cliché is untrue.
-- Stephen Fry

[Regression] Bonding no longer support tun-interfaces

[Jörn Engel]

This has been reported (and ignored) before:
http://lkml.iu.edu/hypermail/linux/kernel/1407.2/03790.html
https://bugzilla.kernel.org/show_bug.cgi?id=89161

Regression was introduced by:

commit 00503b6f702e (refs/bisect/bad)
Author: dingtianhong <dingtianhong@huawei.com>
Date:   Sat Jan 25 13:00:29 2014 +0800

    bonding: fail_over_mac should only affect AB mode at enslave and removal processing
    
    According to bonding.txt, the fail_over_ma should only affect active-backup mode,
    but I found that the fail_over_mac could be set to active or follow in all
    modes, this will cause new slave could not be set to bond's MAC address at
    enslave processing and restore its own MAC address at removal processing.
    
    The correct way to fix the problem is that we should not add restrictions when
    setting options, just need to modify the bond enslave and removal processing
    to check the mode in addition to fail_over_mac when setting a slave's MAC during
    enslavement. The change active slave processing already only calls the fail_over_mac
    function when in active-backup mode.
    
    Thanks for Jay's suggestion.
    
    The patch also modify the pr_warning() to pr_warn().
    
    Cc: Jay Vosburgh <fubar@us.ibm.com>
    Cc: Veaceslav Falico <vfalico@redhat.com>
    Cc: Andy Gospodarek <andy@greyhouse.net>
    Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Since I never needed bonding or tun-interfaces before, I come late to
the party.  Some 6k lines have changed in the bonding driver since the
regression got in two years ago.  So a simple revert is unlikely to lead
to happiness.

But I absolutely need that functionality and would rather run a 3.13
kernel than live with the regression.  dingtianhong, any suggestions?

J�rn

--
It is a clich� that most clich�s are true, but then, like most clich�s,
that clich� is untrue.
-- Stephen Fry

Re: [Regression] Bonding no longer support tun-interfaces

[David Miller]

Report this to the correct mailing list please, networking discussion
occurs on netdev@vger.kernel.org, not linux-kernel or the stable
list.

Thanks.

Re: [Regression] Bonding no longer support tun-interfaces

[Jörn Engel]

Redirected by Davem.

Is there a mailing list or a maintainer for regressions?  There used to
be, but I've been out of the loop for a while.

On Mon, Aug 08, 2016 at 02:15:30PM -0700, Jörn Engel wrote:
> This has been reported (and ignored) before:
> http://lkml.iu.edu/hypermail/linux/kernel/1407.2/03790.html
> https://bugzilla.kernel.org/show_bug.cgi?id=89161
> 
> Regression was introduced by:
> 
> commit 00503b6f702e (refs/bisect/bad)
> Author: dingtianhong <dingtianhong@huawei.com>
> Date:   Sat Jan 25 13:00:29 2014 +0800
> 
>     bonding: fail_over_mac should only affect AB mode at enslave and removal processing
>     
>     According to bonding.txt, the fail_over_ma should only affect active-backup mode,
>     but I found that the fail_over_mac could be set to active or follow in all
>     modes, this will cause new slave could not be set to bond's MAC address at
>     enslave processing and restore its own MAC address at removal processing.
>     
>     The correct way to fix the problem is that we should not add restrictions when
>     setting options, just need to modify the bond enslave and removal processing
>     to check the mode in addition to fail_over_mac when setting a slave's MAC during
>     enslavement. The change active slave processing already only calls the fail_over_mac
>     function when in active-backup mode.
>     
>     Thanks for Jay's suggestion.
>     
>     The patch also modify the pr_warning() to pr_warn().
>     
>     Cc: Jay Vosburgh <fubar@us.ibm.com>
>     Cc: Veaceslav Falico <vfalico@redhat.com>
>     Cc: Andy Gospodarek <andy@greyhouse.net>
>     Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Since I never needed bonding or tun-interfaces before, I come late to
> the party.  Some 6k lines have changed in the bonding driver since the
> regression got in two years ago.  So a simple revert is unlikely to lead
> to happiness.
> 
> But I absolutely need that functionality and would rather run a 3.13
> kernel than live with the regression.  dingtianhong, any suggestions?
> 
> Jörn
> 
> --
> It is a cliché that most clichés are true, but then, like most clichés,
> that cliché is untrue.
> -- Stephen Fry

Jörn

[PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
afaics as an unintended side-effect.

For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
error from dev_set_mac_address() is good enough.  I am not familiar
enough with the code to judge what new problems this patch might
introduce.

Signed-off-by: Joern Engel <joern@purestorage.com>
---
 drivers/net/bonding/bond_main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1f276fa30ba6..bc5dba847f50 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1489,11 +1489,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		 */
 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
 		addr.sa_family = slave_dev->type;
-		res = dev_set_mac_address(slave_dev, &addr);
-		if (res) {
-			netdev_dbg(bond_dev, "Error %d calling set_mac_address\n", res);
-			goto err_restore_mtu;
-		}
+		dev_set_mac_address(slave_dev, &addr);
 	}
 
 	/* set slave flag before open to prevent IPv6 addrconf */
@@ -1777,7 +1773,6 @@ err_restore_mac:
 		dev_set_mac_address(slave_dev, &addr);
 	}
 
-err_restore_mtu:
 	dev_set_mtu(slave_dev, new_slave->original_mtu);
 
 err_free:
-- 
2.1.4

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/9 5:48, Jörn Engel wrote:
> Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
> used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
> afaics as an unintended side-effect.
> 

Hi Jorn:

I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
and we will try to fix it.

and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.

Thanks.
Ding

> For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
> error from dev_set_mac_address() is good enough.  I am not familiar
> enough with the code to judge what new problems this patch might
> introduce.
> 
> Signed-off-by: Joern Engel <joern@purestorage.com>
> ---
>  drivers/net/bonding/bond_main.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..bc5dba847f50 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1489,11 +1489,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  		 */
>  		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
>  		addr.sa_family = slave_dev->type;
> -		res = dev_set_mac_address(slave_dev, &addr);
> -		if (res) {
> -			netdev_dbg(bond_dev, "Error %d calling set_mac_address\n", res);
> -			goto err_restore_mtu;
> -		}
> +		dev_set_mac_address(slave_dev, &addr);
>  	}
>  
>  	/* set slave flag before open to prevent IPv6 addrconf */
> @@ -1777,7 +1773,6 @@ err_restore_mac:
>  		dev_set_mac_address(slave_dev, &addr);
>  	}
>  
> -err_restore_mtu:
>  	dev_set_mtu(slave_dev, new_slave->original_mtu);
>  
>  err_free:
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

Hello Tianhong!

On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
> 
> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
> and we will try to fix it.

Here is a trivial testcase:
openvpn --mktun --dev tun0
echo +tun0 > /sys/class/net/bond0/bonding/slaves

Worked fine before your patch, no longer works after your patch.  Works
again after my patch.

> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.

Using bonding in balance-rr mode, there doesn't seem to be a need to
change the mac address.  I suppose you might care in other modes, but I
don't.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[zhuyj]

Can we check slave_ops->ndo_set_mac_address?

1476         if ((slave_ops->ndo_set_mac_address) &&
(!bond->params.fail_over_mac ||
1477             BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
1478                 /* Set slave to master's mac address. The
application already
1479 * set the master's mac address to that of the first slave
1480 */
1481                 memcpy(addr.sa_data, bond_dev->dev_addr,
bond_dev->addr_len);
1482                 addr.sa_family = slave_dev->type;
1483                 res = dev_set_mac_address(slave_dev, &addr);
1484                 if (res) {
1485                         netdev_dbg(bond_dev, "Error %d calling
set_mac_address\n", res);
1486                         goto err_restore_mtu;
1487                 }
1488         }

On Tue, Aug 9, 2016 at 11:09 AM, Jörn Engel <joern@purestorage.com> wrote:
> Hello Tianhong!
>
> On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
>>
>> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
>> and we will try to fix it.
>
> Here is a trivial testcase:
> openvpn --mktun --dev tun0
> echo +tun0 > /sys/class/net/bond0/bonding/slaves
>
> Worked fine before your patch, no longer works after your patch.  Works
> again after my patch.
>
>> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
>> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.
>
> Using bonding in balance-rr mode, there doesn't seem to be a need to
> change the mac address.  I suppose you might care in other modes, but I
> don't.
>
> Jörn
>
> --
> Time? What's that? Time is only worth what you do with it.
> -- Theo de Raadt

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/9 13:29, zhuyj wrote:
> Can we check slave_ops->ndo_set_mac_address?
> 
> 1476         if ((slave_ops->ndo_set_mac_address) &&
> (!bond->params.fail_over_mac ||
> 1477             BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
> 1478                 /* Set slave to master's mac address. The
> application already
> 1479 * set the master's mac address to that of the first slave
> 1480 */
> 1481                 memcpy(addr.sa_data, bond_dev->dev_addr,
> bond_dev->addr_len);
> 1482                 addr.sa_family = slave_dev->type;
> 1483                 res = dev_set_mac_address(slave_dev, &addr);
> 1484                 if (res) {
> 1485                         netdev_dbg(bond_dev, "Error %d calling
> set_mac_address\n", res);
> 1486                         goto err_restore_mtu;
> 1487                 }
> 1488         }
> 

This patch is a simple solution for this problem, but I don't think it is the right solution, the bond is a virtual device base on L2,
if the slave has no mac address, it will break the design principle, so we need to think more about it.

I think if the bonding dev has to support L3 virtual device, we need to add new bond features to distinguish the dev and make the
bond xmit and transfer without the mac address.

Thanks
Ding

> On Tue, Aug 9, 2016 at 11:09 AM, Jörn Engel <joern@purestorage.com> wrote:
>> Hello Tianhong!
>>
>> On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
>>>
>>> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
>>> and we will try to fix it.
>>
>> Here is a trivial testcase:
>> openvpn --mktun --dev tun0
>> echo +tun0 > /sys/class/net/bond0/bonding/slaves
>>
>> Worked fine before your patch, no longer works after your patch.  Works
>> again after my patch.
>>
>>> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
>>> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.
>>
>> Using bonding in balance-rr mode, there doesn't seem to be a need to
>> change the mac address.  I suppose you might care in other modes, but I
>> don't.
>>
>> Jörn
>>
>> --
>> Time? What's that? Time is only worth what you do with it.
>> -- Theo de Raadt
> 
> .
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
> 
> This patch is a simple solution for this problem, but I don't think it is the right solution, the bond is a virtual device base on L2,
> if the slave has no mac address, it will break the design principle, so we need to think more about it.

The important point is: it worked.  It solved a problem that at least
three people cared enough about to send a bug report.

Now it doesn't work anymore.  That is a regression.

Whether or not L2 has always been a design principle for bonding can be
argued as well.  But in the face of a regression, I suggest we fix the
regression.

> I think if the bonding dev has to support L3 virtual device, we need to add new bond features to distinguish the dev and make the
> bond xmit and transfer without the mac address.

Simply not checking errors when setting the mac address solves the
problem for me.  No new features needed.

If you want to retain error handling, you can make those checks
conditional on the mode.  In balance-rr or broadcast mode, ignore the
error.  I don't need and haven't tested broadcast mode, but it doesn't
seem to depend on any L2 attributes either.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[David Miller]

From: Jörn Engel <joern@purestorage.com>
Date: Tue, 9 Aug 2016 11:08:30 -0700

> On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>> 
>> I think if the bonding dev has to support L3 virtual device, we need to add new bond features to distinguish the dev and make the
>> bond xmit and transfer without the mac address.
> 
> Simply not checking errors when setting the mac address solves the
> problem for me.  No new features needed.

But it only works in certain modes.

So the best we can do is enforce the MAC address setting in the
modes that absolutely require it.  We cannot ignore the MAC
address setting unilaterally.

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
> > On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
> > 
> > Simply not checking errors when setting the mac address solves the
> > problem for me.  No new features needed.
> 
> But it only works in certain modes.
> 
> So the best we can do is enforce the MAC address setting in the
> modes that absolutely require it.  We cannot ignore the MAC
> address setting unilaterally.

Something like this?

[PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode

Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
afaics as an unintended side-effect.

For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
error from dev_set_mac_address() is good enough.

Signed-off-by: Joern Engel <joern@purestorage.com>
---
 drivers/net/bonding/bond_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1f276fa30ba6..2f686bfe4304 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
 		addr.sa_family = slave_dev->type;
 		res = dev_set_mac_address(slave_dev, &addr);
-		if (res) {
+		/* round-robin mode works fine without a mac address */
+		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {
 			netdev_dbg(bond_dev, "Error %d calling set_mac_address\n", res);
 			goto err_restore_mtu;
 		}
-- 
2.1.4

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jay Vosburgh]

Jörn Engel <joern@purestorage.com> wrote:

>On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
>> > On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>> > 
>> > Simply not checking errors when setting the mac address solves the
>> > problem for me.  No new features needed.
>> 
>> But it only works in certain modes.
>> 
>> So the best we can do is enforce the MAC address setting in the
>> modes that absolutely require it.  We cannot ignore the MAC
>> address setting unilaterally.
>
>Something like this?
>
>[PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode
>
>Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
>used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
>afaics as an unintended side-effect.
>
>For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
>error from dev_set_mac_address() is good enough.
>
>Signed-off-by: Joern Engel <joern@purestorage.com>
>---
> drivers/net/bonding/bond_main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1f276fa30ba6..2f686bfe4304 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
> 		addr.sa_family = slave_dev->type;
> 		res = dev_set_mac_address(slave_dev, &addr);
>-		if (res) {
>+		/* round-robin mode works fine without a mac address */
>+		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {

	This will cause balance-rr to add the slave to the bond if any
device's dev_set_mac_address call fails.

	If a bond of regular Ethernet devices is connected to a static
link aggregation (Etherchannel channel group), a set_mac failure would
result in that slave having a different MAC address than the bond, which
in turn would cause traffic inbound from the switch to that slave to be
dropped (as the destination MAC would not pass the device MAC filters).

	The failure check for the set_mac call serves a legitimate
purpose, and I don't believe we should bypass it without making the
bypass an option that is explicitly enabled for those special cases that
need it.

	E.g., something like the following (which I have not tested);
this would also need documentation and iproute2 updates to go with it.
This would be enabled with "fail_over_mac=keepmac".

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1f276fa30ba6..d2283fc23b16 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1483,7 +1483,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
 
 	if (!bond->params.fail_over_mac ||
-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
+	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
 		/* Set slave to master's mac address.  The application already
 		 * set the master's mac address to that of the first slave
 		 */
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 577e57cad1dc..f9653fe4d622 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
 	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
 	{ "active", BOND_FOM_ACTIVE, 0},
 	{ "follow", BOND_FOM_FOLLOW, 0},
+	{ "keepmac", BOND_FOM_KEEPMAC, 0},
 	{ NULL,     -1,              0},
 };
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 6360c259da6d..ec3442b3aa83 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
 #define BOND_FOM_NONE			0
 #define BOND_FOM_ACTIVE			1
 #define BOND_FOM_FOLLOW			2
+#define BOND_FOM_KEEPMAC		3
 
 #define BOND_ARP_TARGETS_ANY		0
 #define BOND_ARP_TARGETS_ALL		1


	-J

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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/10 7:51, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> 
>> On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
>>>> On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>>>>
>>>> Simply not checking errors when setting the mac address solves the
>>>> problem for me.  No new features needed.
>>>
>>> But it only works in certain modes.
>>>
>>> So the best we can do is enforce the MAC address setting in the
>>> modes that absolutely require it.  We cannot ignore the MAC
>>> address setting unilaterally.
>>
>> Something like this?
>>
>> [PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode
>>
>> Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
>> used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
>> afaics as an unintended side-effect.
>>
>> For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
>> error from dev_set_mac_address() is good enough.
>>
>> Signed-off-by: Joern Engel <joern@purestorage.com>
>> ---
>> drivers/net/bonding/bond_main.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..2f686bfe4304 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
>> 		addr.sa_family = slave_dev->type;
>> 		res = dev_set_mac_address(slave_dev, &addr);
>> -		if (res) {
>> +		/* round-robin mode works fine without a mac address */
>> +		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {
> 
> 	This will cause balance-rr to add the slave to the bond if any
> device's dev_set_mac_address call fails.
> 
> 	If a bond of regular Ethernet devices is connected to a static
> link aggregation (Etherchannel channel group), a set_mac failure would
> result in that slave having a different MAC address than the bond, which
> in turn would cause traffic inbound from the switch to that slave to be
> dropped (as the destination MAC would not pass the device MAC filters).
> 
> 	The failure check for the set_mac call serves a legitimate
> purpose, and I don't believe we should bypass it without making the
> bypass an option that is explicitly enabled for those special cases that
> need it.
> 
> 	E.g., something like the following (which I have not tested);
> this would also need documentation and iproute2 updates to go with it.
> This would be enabled with "fail_over_mac=keepmac".
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..d2283fc23b16 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1483,7 +1483,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>  
>  	if (!bond->params.fail_over_mac ||
> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> +	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
>  		/* Set slave to master's mac address.  The application already
>  		 * set the master's mac address to that of the first slave
>  		 */
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 577e57cad1dc..f9653fe4d622 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
>  	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
>  	{ "active", BOND_FOM_ACTIVE, 0},
>  	{ "follow", BOND_FOM_FOLLOW, 0},
> +	{ "keepmac", BOND_FOM_KEEPMAC, 0},
>  	{ NULL,     -1,              0},
>  };
>  
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 6360c259da6d..ec3442b3aa83 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
>  #define BOND_FOM_NONE			0
>  #define BOND_FOM_ACTIVE			1
>  #define BOND_FOM_FOLLOW			2
> +#define BOND_FOM_KEEPMAC		3
>  
>  #define BOND_ARP_TARGETS_ANY		0
>  #define BOND_ARP_TARGETS_ALL		1
> 
> 
> 	-J
> 

Hi Jay:

It looks the best solution till now, the user need to ensure the slave don't need the same mac
any more, and no need to checking the ndo_set_mac_address, it looks need more think about this later,
but let we fix it first.

Ding

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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/10 7:51, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> 
>> On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
>>>> On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>>>>
>>>> Simply not checking errors when setting the mac address solves the
>>>> problem for me.  No new features needed.
>>>
>>> But it only works in certain modes.
>>>
>>> So the best we can do is enforce the MAC address setting in the
>>> modes that absolutely require it.  We cannot ignore the MAC
>>> address setting unilaterally.
>>
>> Something like this?
>>
>> [PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode
>>
>> Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
>> used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
>> afaics as an unintended side-effect.
>>
>> For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
>> error from dev_set_mac_address() is good enough.
>>
>> Signed-off-by: Joern Engel <joern@purestorage.com>
>> ---
>> drivers/net/bonding/bond_main.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..2f686bfe4304 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
>> 		addr.sa_family = slave_dev->type;
>> 		res = dev_set_mac_address(slave_dev, &addr);
>> -		if (res) {
>> +		/* round-robin mode works fine without a mac address */
>> +		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {
> 
> 	This will cause balance-rr to add the slave to the bond if any
> device's dev_set_mac_address call fails.
> 
> 	If a bond of regular Ethernet devices is connected to a static
> link aggregation (Etherchannel channel group), a set_mac failure would
> result in that slave having a different MAC address than the bond, which
> in turn would cause traffic inbound from the switch to that slave to be
> dropped (as the destination MAC would not pass the device MAC filters).
> 
> 	The failure check for the set_mac call serves a legitimate
> purpose, and I don't believe we should bypass it without making the
> bypass an option that is explicitly enabled for those special cases that
> need it.
> 
> 	E.g., something like the following (which I have not tested);
> this would also need documentation and iproute2 updates to go with it.
> This would be enabled with "fail_over_mac=keepmac".
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..d2283fc23b16 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1483,7 +1483,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>  
>  	if (!bond->params.fail_over_mac ||
> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> +	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
>  		/* Set slave to master's mac address.  The application already
>  		 * set the master's mac address to that of the first slave
>  		 */
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 577e57cad1dc..f9653fe4d622 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
>  	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
>  	{ "active", BOND_FOM_ACTIVE, 0},
>  	{ "follow", BOND_FOM_FOLLOW, 0},
> +	{ "keepmac", BOND_FOM_KEEPMAC, 0},
>  	{ NULL,     -1,              0},
>  };
>  
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 6360c259da6d..ec3442b3aa83 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
>  #define BOND_FOM_NONE			0
>  #define BOND_FOM_ACTIVE			1
>  #define BOND_FOM_FOLLOW			2
> +#define BOND_FOM_KEEPMAC		3
>  
>  #define BOND_ARP_TARGETS_ANY		0
>  #define BOND_ARP_TARGETS_ALL		1
> 
> 
> 	-J
> 
Hi jorn:

Could you please test this patch? I build this patch base on Jay's suggestion and I think it could fix your problem.

---
 drivers/net/bonding/bond_main.c    | 24 +++++++++---------------
 drivers/net/bonding/bond_options.c |  3 ++-
 include/net/bonding.h              |  1 +
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1f276fa..dd4a8eb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -174,9 +174,9 @@ MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes; "
 module_param(arp_all_targets, charp, 0);
 MODULE_PARM_DESC(arp_all_targets, "fail on any/all arp targets timeout; 0 for any (default), 1 for all");
 module_param(fail_over_mac, charp, 0);
-MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to "
-				"the same MAC; 0 for none (default), "
-				"1 for active, 2 for follow");
+MODULE_PARM_DESC(fail_over_mac, "Do not set all slaves to the same MAC; "
+				"0 for none (default), 1 for active, "
+				"2 for follow, 3 for keepmac");
 module_param(all_slaves_active, int, 0);
 MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface "
 				     "by setting active flag for all slaves; "
@@ -1482,8 +1482,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	 */
 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
 
-	if (!bond->params.fail_over_mac ||
-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+	if (bond->params.fail_over_mac == BOND_FOM_NONE) {
 		/* Set slave to master's mac address.  The application already
 		 * set the master's mac address to that of the first slave
 		 */
@@ -1766,8 +1765,7 @@ err_close:
 
 err_restore_mac:
 	slave_dev->flags &= ~IFF_SLAVE;
-	if (!bond->params.fail_over_mac ||
-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+	if (bond->params.fail_over_mac == BOND_FOM_NONE) {
 		/* XXX TODO - fom follow mode needs to change master's
 		 * MAC if this slave's MAC is in use by the bond, or at
 		 * least print a warning.
@@ -1867,8 +1865,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 
 	RCU_INIT_POINTER(bond->current_arp_slave, NULL);
 
-	if (!all && (!bond->params.fail_over_mac ||
-		     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
+	if (!all && bond->params.fail_over_mac == BOND_FOM_NONE) {
 		if (ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
 		    bond_has_slaves(bond))
 			netdev_warn(bond_dev, "the permanent HWaddr of %s - %pM - is still in use by %s - set the HWaddr of %s to a different address to avoid conflicts\n",
@@ -1949,8 +1946,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 	/* close slave before restoring its mac address */
 	dev_close(slave_dev);
 
-	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+	if (bond->params.fail_over_mac == BOND_FOM_FOLLOW ||
+	    bond->params.fail_over_mac == BOND_FOM_NONE) {
 		/* restore original ("permanent") mac address */
 		ether_addr_copy(addr.sa_data, slave->perm_hwaddr);
 		addr.sa_family = slave_dev->type;
@@ -3634,8 +3631,7 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
 	/* If fail_over_mac is enabled, do nothing and return success.
 	 * Returning an error causes ifenslave to fail.
 	 */
-	if (bond->params.fail_over_mac &&
-	    BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
+	if (bond->params.fail_over_mac)
 		return 0;
 
 	if (!is_valid_ether_addr(sa->sa_data))
@@ -4543,8 +4539,6 @@ static int bond_check_params(struct bond_params *params)
 			return -EINVAL;
 		}
 		fail_over_mac_value = valptr->value;
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
-			pr_warn("Warning: fail_over_mac only affects active-backup mode\n");
 	} else {
 		fail_over_mac_value = BOND_FOM_NONE;
 	}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 577e57c..9038be5 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
 	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
 	{ "active", BOND_FOM_ACTIVE, 0},
 	{ "follow", BOND_FOM_FOLLOW, 0},
+	{ "keepmac", BOND_FOM_KEEPMAC, 0},
 	{ NULL,     -1,              0},
 };
 
@@ -247,7 +248,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
 	[BOND_OPT_FAIL_OVER_MAC] = {
 		.id = BOND_OPT_FAIL_OVER_MAC,
 		.name = "fail_over_mac",
-		.desc = "For active-backup, do not set all slaves to the same MAC",
+		.desc = "Do not set all slaves to the same MAC",
 		.flags = BOND_OPTFLAG_NOSLAVES,
 		.values = bond_fail_over_mac_tbl,
 		.set = bond_option_fail_over_mac_set
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 6360c25..ec3442b 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
 #define BOND_FOM_NONE			0
 #define BOND_FOM_ACTIVE			1
 #define BOND_FOM_FOLLOW			2
+#define BOND_FOM_KEEPMAC		3
 
 #define BOND_ARP_TARGETS_ANY		0
 #define BOND_ARP_TARGETS_ALL		1
-- 
1.9.0




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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>On 2016/8/10 7:51, Jay Vosburgh wrote:
>> Jörn Engel <joern@purestorage.com> wrote:
>> 
>>> On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
>>>>> On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>>>>>
>>>>> Simply not checking errors when setting the mac address solves the
>>>>> problem for me.  No new features needed.
>>>>
>>>> But it only works in certain modes.
>>>>
>>>> So the best we can do is enforce the MAC address setting in the
>>>> modes that absolutely require it.  We cannot ignore the MAC
>>>> address setting unilaterally.
>>>
>>> Something like this?
>>>
>>> [PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode
>>>
>>> Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
>>> used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
>>> afaics as an unintended side-effect.
>>>
>>> For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
>>> error from dev_set_mac_address() is good enough.
>>>
>>> Signed-off-by: Joern Engel <joern@purestorage.com>
>>> ---
>>> drivers/net/bonding/bond_main.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 1f276fa30ba6..2f686bfe4304 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>> 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
>>> 		addr.sa_family = slave_dev->type;
>>> 		res = dev_set_mac_address(slave_dev, &addr);
>>> -		if (res) {
>>> +		/* round-robin mode works fine without a mac address */
>>> +		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {
>> 
>> 	This will cause balance-rr to add the slave to the bond if any
>> device's dev_set_mac_address call fails.
>> 
>> 	If a bond of regular Ethernet devices is connected to a static
>> link aggregation (Etherchannel channel group), a set_mac failure would
>> result in that slave having a different MAC address than the bond, which
>> in turn would cause traffic inbound from the switch to that slave to be
>> dropped (as the destination MAC would not pass the device MAC filters).
>> 
>> 	The failure check for the set_mac call serves a legitimate
>> purpose, and I don't believe we should bypass it without making the
>> bypass an option that is explicitly enabled for those special cases that
>> need it.
>> 
>> 	E.g., something like the following (which I have not tested);
>> this would also need documentation and iproute2 updates to go with it.
>> This would be enabled with "fail_over_mac=keepmac".
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..d2283fc23b16 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1483,7 +1483,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>  	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>>  
>>  	if (!bond->params.fail_over_mac ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
>> +	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
>>  		/* Set slave to master's mac address.  The application already
>>  		 * set the master's mac address to that of the first slave
>>  		 */
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index 577e57cad1dc..f9653fe4d622 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
>>  	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
>>  	{ "active", BOND_FOM_ACTIVE, 0},
>>  	{ "follow", BOND_FOM_FOLLOW, 0},
>> +	{ "keepmac", BOND_FOM_KEEPMAC, 0},
>>  	{ NULL,     -1,              0},
>>  };
>>  
>> diff --git a/include/net/bonding.h b/include/net/bonding.h
>> index 6360c259da6d..ec3442b3aa83 100644
>> --- a/include/net/bonding.h
>> +++ b/include/net/bonding.h
>> @@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
>>  #define BOND_FOM_NONE			0
>>  #define BOND_FOM_ACTIVE			1
>>  #define BOND_FOM_FOLLOW			2
>> +#define BOND_FOM_KEEPMAC		3
>>  
>>  #define BOND_ARP_TARGETS_ANY		0
>>  #define BOND_ARP_TARGETS_ALL		1
>> 
>> 
>> 	-J
>> 
>Hi jorn:
>
>Could you please test this patch? I build this patch base on Jay's suggestion and I think it could fix your problem.
>
>---
> drivers/net/bonding/bond_main.c    | 24 +++++++++---------------
> drivers/net/bonding/bond_options.c |  3 ++-
> include/net/bonding.h              |  1 +
> 3 files changed, 12 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1f276fa..dd4a8eb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -174,9 +174,9 @@ MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes; "
> module_param(arp_all_targets, charp, 0);
> MODULE_PARM_DESC(arp_all_targets, "fail on any/all arp targets timeout; 0 for any (default), 1 for all");
> module_param(fail_over_mac, charp, 0);
>-MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to "
>-				"the same MAC; 0 for none (default), "
>-				"1 for active, 2 for follow");
>+MODULE_PARM_DESC(fail_over_mac, "Do not set all slaves to the same MAC; "
>+				"0 for none (default), 1 for active, "
>+				"2 for follow, 3 for keepmac");
> module_param(all_slaves_active, int, 0);
> MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface "
> 				     "by setting active flag for all slaves; "
>@@ -1482,8 +1482,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	 */
> 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
> 
>-	if (!bond->params.fail_over_mac ||
>-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>+	if (bond->params.fail_over_mac == BOND_FOM_NONE) {

	I'm not sure we can make the change this way; I structured the
test as:

 	if (!bond->params.fail_over_mac ||
-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
+	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {

	deliberately, because the current implementation permits setting
f_o_m at any time, but it only takes effect in active-backup mode.  This
is done to reduce ordering restrictions when setting up the bond,
however, a user today could set f_o_m to "active" or "follow" in modes
other than active-backup and it would have no effect, but your change
would cause that identically configured bond to now behave differently.

	The test has to be structured such that f_o_m set to "active" or
"follow" affects only active-backup mode, and f_o_m "keepmac" affects
any mode.  We probably should move this into a helper, something like

bool bond_fom_enabled(bond)
{
	if (bond->mode == BOND_MODE_ACTIVEBACKUP)
		return bond->params.fail_over_mac != BOND_FOM_NONE;
	else
		return bond->params.fail_over_mac == BOND_FOM_KEEPMAC;
}

	then the complicated test becomes

	if (!bond_fom_enabled(bond)) {
		[ change the MAC address stuff ]
	}


	-J

> 		/* Set slave to master's mac address.  The application already
> 		 * set the master's mac address to that of the first slave
> 		 */
>@@ -1766,8 +1765,7 @@ err_close:
> 
> err_restore_mac:
> 	slave_dev->flags &= ~IFF_SLAVE;
>-	if (!bond->params.fail_over_mac ||
>-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>+	if (bond->params.fail_over_mac == BOND_FOM_NONE) {
> 		/* XXX TODO - fom follow mode needs to change master's
> 		 * MAC if this slave's MAC is in use by the bond, or at
> 		 * least print a warning.
>@@ -1867,8 +1865,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> 
> 	RCU_INIT_POINTER(bond->current_arp_slave, NULL);
> 
>-	if (!all && (!bond->params.fail_over_mac ||
>-		     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
>+	if (!all && bond->params.fail_over_mac == BOND_FOM_NONE) {
> 		if (ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
> 		    bond_has_slaves(bond))
> 			netdev_warn(bond_dev, "the permanent HWaddr of %s - %pM - is still in use by %s - set the HWaddr of %s to a different address to avoid conflicts\n",
>@@ -1949,8 +1946,8 @@ static int __bond_release_one(struct net_device *bond_dev,
> 	/* close slave before restoring its mac address */
> 	dev_close(slave_dev);
> 
>-	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
>-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>+	if (bond->params.fail_over_mac == BOND_FOM_FOLLOW ||
>+	    bond->params.fail_over_mac == BOND_FOM_NONE) {
> 		/* restore original ("permanent") mac address */
> 		ether_addr_copy(addr.sa_data, slave->perm_hwaddr);
> 		addr.sa_family = slave_dev->type;
>@@ -3634,8 +3631,7 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
> 	/* If fail_over_mac is enabled, do nothing and return success.
> 	 * Returning an error causes ifenslave to fail.
> 	 */
>-	if (bond->params.fail_over_mac &&
>-	    BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
>+	if (bond->params.fail_over_mac)
> 		return 0;
> 
> 	if (!is_valid_ether_addr(sa->sa_data))
>@@ -4543,8 +4539,6 @@ static int bond_check_params(struct bond_params *params)
> 			return -EINVAL;
> 		}
> 		fail_over_mac_value = valptr->value;
>-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
>-			pr_warn("Warning: fail_over_mac only affects active-backup mode\n");
> 	} else {
> 		fail_over_mac_value = BOND_FOM_NONE;
> 	}
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 577e57c..9038be5 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
> 	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
> 	{ "active", BOND_FOM_ACTIVE, 0},
> 	{ "follow", BOND_FOM_FOLLOW, 0},
>+	{ "keepmac", BOND_FOM_KEEPMAC, 0},
> 	{ NULL,     -1,              0},
> };
> 
>@@ -247,7 +248,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
> 	[BOND_OPT_FAIL_OVER_MAC] = {
> 		.id = BOND_OPT_FAIL_OVER_MAC,
> 		.name = "fail_over_mac",
>-		.desc = "For active-backup, do not set all slaves to the same MAC",
>+		.desc = "Do not set all slaves to the same MAC",
> 		.flags = BOND_OPTFLAG_NOSLAVES,
> 		.values = bond_fail_over_mac_tbl,
> 		.set = bond_option_fail_over_mac_set
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 6360c25..ec3442b 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
> #define BOND_FOM_NONE			0
> #define BOND_FOM_ACTIVE			1
> #define BOND_FOM_FOLLOW			2
>+#define BOND_FOM_KEEPMAC		3
> 
> #define BOND_ARP_TARGETS_ANY		0
> #define BOND_ARP_TARGETS_ALL		1
>-- 
>1.9.0

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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/11 1:41, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> On 2016/8/10 7:51, Jay Vosburgh wrote:
>>> Jörn Engel <joern@purestorage.com> wrote:
>>>
>>>> On Tue, Aug 09, 2016 at 12:06:36PM -0700, David Miller wrote:
>>>>>> On Tue, Aug 09, 2016 at 09:28:45PM +0800, Ding Tianhong wrote:
>>>>>>
>>>>>> Simply not checking errors when setting the mac address solves the
>>>>>> problem for me.  No new features needed.
>>>>>
>>>>> But it only works in certain modes.
>>>>>
>>>>> So the best we can do is enforce the MAC address setting in the
>>>>> modes that absolutely require it.  We cannot ignore the MAC
>>>>> address setting unilaterally.
>>>>
>>>> Something like this?
>>>>
>>>> [PATCH] bonding: Allow tun-interfaces as slaves in balance-rr mode
>>>>
>>>> Up until 00503b6f702e (part of 3.14-rc1), the bonding driver could be
>>>> used to enslave tun-interfaces.  00503b6f702e broke that behaviour,
>>>> afaics as an unintended side-effect.
>>>>
>>>> For the purpose of bond-over-tun in balance-rr mode, simply ignoring the
>>>> error from dev_set_mac_address() is good enough.
>>>>
>>>> Signed-off-by: Joern Engel <joern@purestorage.com>
>>>> ---
>>>> drivers/net/bonding/bond_main.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>> index 1f276fa30ba6..2f686bfe4304 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -1490,7 +1490,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>>> 		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
>>>> 		addr.sa_family = slave_dev->type;
>>>> 		res = dev_set_mac_address(slave_dev, &addr);
>>>> -		if (res) {
>>>> +		/* round-robin mode works fine without a mac address */
>>>> +		if (res && BOND_MODE(bond) != BOND_MODE_ROUNDROBIN) {
>>>
>>> 	This will cause balance-rr to add the slave to the bond if any
>>> device's dev_set_mac_address call fails.
>>>
>>> 	If a bond of regular Ethernet devices is connected to a static
>>> link aggregation (Etherchannel channel group), a set_mac failure would
>>> result in that slave having a different MAC address than the bond, which
>>> in turn would cause traffic inbound from the switch to that slave to be
>>> dropped (as the destination MAC would not pass the device MAC filters).
>>>
>>> 	The failure check for the set_mac call serves a legitimate
>>> purpose, and I don't believe we should bypass it without making the
>>> bypass an option that is explicitly enabled for those special cases that
>>> need it.
>>>
>>> 	E.g., something like the following (which I have not tested);
>>> this would also need documentation and iproute2 updates to go with it.
>>> This would be enabled with "fail_over_mac=keepmac".
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 1f276fa30ba6..d2283fc23b16 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1483,7 +1483,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>>  	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>>>  
>>>  	if (!bond->params.fail_over_mac ||
>>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>>> +	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
>>> +	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
>>>  		/* Set slave to master's mac address.  The application already
>>>  		 * set the master's mac address to that of the first slave
>>>  		 */
>>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>>> index 577e57cad1dc..f9653fe4d622 100644
>>> --- a/drivers/net/bonding/bond_options.c
>>> +++ b/drivers/net/bonding/bond_options.c
>>> @@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
>>>  	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
>>>  	{ "active", BOND_FOM_ACTIVE, 0},
>>>  	{ "follow", BOND_FOM_FOLLOW, 0},
>>> +	{ "keepmac", BOND_FOM_KEEPMAC, 0},
>>>  	{ NULL,     -1,              0},
>>>  };
>>>  
>>> diff --git a/include/net/bonding.h b/include/net/bonding.h
>>> index 6360c259da6d..ec3442b3aa83 100644
>>> --- a/include/net/bonding.h
>>> +++ b/include/net/bonding.h
>>> @@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
>>>  #define BOND_FOM_NONE			0
>>>  #define BOND_FOM_ACTIVE			1
>>>  #define BOND_FOM_FOLLOW			2
>>> +#define BOND_FOM_KEEPMAC		3
>>>  
>>>  #define BOND_ARP_TARGETS_ANY		0
>>>  #define BOND_ARP_TARGETS_ALL		1
>>>
>>>
>>> 	-J
>>>
>> Hi jorn:
>>
>> Could you please test this patch? I build this patch base on Jay's suggestion and I think it could fix your problem.
>>
>> ---
>> drivers/net/bonding/bond_main.c    | 24 +++++++++---------------
>> drivers/net/bonding/bond_options.c |  3 ++-
>> include/net/bonding.h              |  1 +
>> 3 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa..dd4a8eb 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -174,9 +174,9 @@ MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes; "
>> module_param(arp_all_targets, charp, 0);
>> MODULE_PARM_DESC(arp_all_targets, "fail on any/all arp targets timeout; 0 for any (default), 1 for all");
>> module_param(fail_over_mac, charp, 0);
>> -MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to "
>> -				"the same MAC; 0 for none (default), "
>> -				"1 for active, 2 for follow");
>> +MODULE_PARM_DESC(fail_over_mac, "Do not set all slaves to the same MAC; "
>> +				"0 for none (default), 1 for active, "
>> +				"2 for follow, 3 for keepmac");
>> module_param(all_slaves_active, int, 0);
>> MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface "
>> 				     "by setting active flag for all slaves; "
>> @@ -1482,8 +1482,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 	 */
>> 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>>
>> -	if (!bond->params.fail_over_mac ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	if (bond->params.fail_over_mac == BOND_FOM_NONE) {
> 
> 	I'm not sure we can make the change this way; I structured the
> test as:
> 
>  	if (!bond->params.fail_over_mac ||
> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> +	    (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +	     bond->params.fail_over_mac != BOND_FOM_KEEPMAC)) {
> 
> 	deliberately, because the current implementation permits setting
> f_o_m at any time, but it only takes effect in active-backup mode.  This
> is done to reduce ordering restrictions when setting up the bond,
> however, a user today could set f_o_m to "active" or "follow" in modes
> other than active-backup and it would have no effect, but your change
> would cause that identically configured bond to now behave differently.
> 

OK, it looks no need to restrict other mode(no active-backup mode) to work with active or follow policy,
no effect is enough.

> 	The test has to be structured such that f_o_m set to "active" or
> "follow" affects only active-backup mode, and f_o_m "keepmac" affects
> any mode.  We probably should move this into a helper, something like
> 
> bool bond_fom_enabled(bond)
> {
> 	if (bond->mode == BOND_MODE_ACTIVEBACKUP)
> 		return bond->params.fail_over_mac != BOND_FOM_NONE;
> 	else
> 		return bond->params.fail_over_mac == BOND_FOM_KEEPMAC;
> }
> 

fine.

Thanks.
Ding

> 	then the complicated test becomes
> 
> 	if (!bond_fom_enabled(bond)) {
> 		[ change the MAC address stuff ]
> 	}
> 
> 
> 	-J
> 
>> 		/* Set slave to master's mac address.  The application already
>> 		 * set the master's mac address to that of the first slave
>> 		 */
>> @@ -1766,8 +1765,7 @@ err_close:
>>
>> err_restore_mac:
>> 	slave_dev->flags &= ~IFF_SLAVE;
>> -	if (!bond->params.fail_over_mac ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	if (bond->params.fail_over_mac == BOND_FOM_NONE) {
>> 		/* XXX TODO - fom follow mode needs to change master's
>> 		 * MAC if this slave's MAC is in use by the bond, or at
>> 		 * least print a warning.
>> @@ -1867,8 +1865,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>>
>> 	RCU_INIT_POINTER(bond->current_arp_slave, NULL);
>>
>> -	if (!all && (!bond->params.fail_over_mac ||
>> -		     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
>> +	if (!all && bond->params.fail_over_mac == BOND_FOM_NONE) {
>> 		if (ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
>> 		    bond_has_slaves(bond))
>> 			netdev_warn(bond_dev, "the permanent HWaddr of %s - %pM - is still in use by %s - set the HWaddr of %s to a different address to avoid conflicts\n",
>> @@ -1949,8 +1946,8 @@ static int __bond_release_one(struct net_device *bond_dev,
>> 	/* close slave before restoring its mac address */
>> 	dev_close(slave_dev);
>>
>> -	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	if (bond->params.fail_over_mac == BOND_FOM_FOLLOW ||
>> +	    bond->params.fail_over_mac == BOND_FOM_NONE) {
>> 		/* restore original ("permanent") mac address */
>> 		ether_addr_copy(addr.sa_data, slave->perm_hwaddr);
>> 		addr.sa_family = slave_dev->type;
>> @@ -3634,8 +3631,7 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
>> 	/* If fail_over_mac is enabled, do nothing and return success.
>> 	 * Returning an error causes ifenslave to fail.
>> 	 */
>> -	if (bond->params.fail_over_mac &&
>> -	    BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
>> +	if (bond->params.fail_over_mac)
>> 		return 0;
>>
>> 	if (!is_valid_ether_addr(sa->sa_data))
>> @@ -4543,8 +4539,6 @@ static int bond_check_params(struct bond_params *params)
>> 			return -EINVAL;
>> 		}
>> 		fail_over_mac_value = valptr->value;
>> -		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
>> -			pr_warn("Warning: fail_over_mac only affects active-backup mode\n");
>> 	} else {
>> 		fail_over_mac_value = BOND_FOM_NONE;
>> 	}
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index 577e57c..9038be5 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -125,6 +125,7 @@ static const struct bond_opt_value bond_fail_over_mac_tbl[] = {
>> 	{ "none",   BOND_FOM_NONE,   BOND_VALFLAG_DEFAULT},
>> 	{ "active", BOND_FOM_ACTIVE, 0},
>> 	{ "follow", BOND_FOM_FOLLOW, 0},
>> +	{ "keepmac", BOND_FOM_KEEPMAC, 0},
>> 	{ NULL,     -1,              0},
>> };
>>
>> @@ -247,7 +248,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
>> 	[BOND_OPT_FAIL_OVER_MAC] = {
>> 		.id = BOND_OPT_FAIL_OVER_MAC,
>> 		.name = "fail_over_mac",
>> -		.desc = "For active-backup, do not set all slaves to the same MAC",
>> +		.desc = "Do not set all slaves to the same MAC",
>> 		.flags = BOND_OPTFLAG_NOSLAVES,
>> 		.values = bond_fail_over_mac_tbl,
>> 		.set = bond_option_fail_over_mac_set
>> diff --git a/include/net/bonding.h b/include/net/bonding.h
>> index 6360c25..ec3442b 100644
>> --- a/include/net/bonding.h
>> +++ b/include/net/bonding.h
>> @@ -420,6 +420,7 @@ static inline bool bond_slave_can_tx(struct slave *slave)
>> #define BOND_FOM_NONE			0
>> #define BOND_FOM_ACTIVE			1
>> #define BOND_FOM_FOLLOW			2
>> +#define BOND_FOM_KEEPMAC		3
>>
>> #define BOND_ARP_TARGETS_ANY		0
>> #define BOND_ARP_TARGETS_ALL		1
>> -- 
>> 1.9.0
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> 
> .
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Tue, Aug 09, 2016 at 04:51:04PM -0700, Jay Vosburgh wrote:
> 
> 	This will cause balance-rr to add the slave to the bond if any
> device's dev_set_mac_address call fails.
> 
> 	If a bond of regular Ethernet devices is connected to a static
> link aggregation (Etherchannel channel group), a set_mac failure would
> result in that slave having a different MAC address than the bond, which
> in turn would cause traffic inbound from the switch to that slave to be
> dropped (as the destination MAC would not pass the device MAC filters).
> 
> 	The failure check for the set_mac call serves a legitimate
> purpose, and I don't believe we should bypass it without making the
> bypass an option that is explicitly enabled for those special cases that
> need it.
> 
> 	E.g., something like the following (which I have not tested);
> this would also need documentation and iproute2 updates to go with it.
> This would be enabled with "fail_over_mac=keepmac".

Thank you!

Tested-by: Jörn Engel <joern@purestorage.com>

Having to set one more parameter is a bit annoying.  It would have to be
documented in a prominent place and people would still often miss it.
So I wonder if we can make the interface a little nicer.

Options:
- If there are no slaves yet and the first slave added is tun, we trust
  the users to know what they are doing.  Automatically set
  bond->params.fail_over_mac = BOND_FOM_KEEPMAC
  Maybe do a printk to inform the user in case of a mistake.
- If we get an error and the slave device is tun, do a printk giving the
  user enough information to find this parameter.

I'm leaning towards the former, but you probably know a reason why I am
wrong again.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
> 
> Having to set one more parameter is a bit annoying.  It would have to be
> documented in a prominent place and people would still often miss it.
> So I wonder if we can make the interface a little nicer.
> 
> Options:
> - If there are no slaves yet and the first slave added is tun, we trust
>   the users to know what they are doing.  Automatically set
>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
>   Maybe do a printk to inform the user in case of a mistake.
> - If we get an error and the slave device is tun, do a printk giving the
>   user enough information to find this parameter.
> 
> I'm leaning towards the former, but you probably know a reason why I am
> wrong again.

Patch below is an implementation of the former.  Not sure if something
like this is worth considering.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jay Vosburgh]

Jörn Engel <joern@purestorage.com> wrote:

>On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
>> 
>> Having to set one more parameter is a bit annoying.  It would have to be
>> documented in a prominent place and people would still often miss it.
>> So I wonder if we can make the interface a little nicer.
>> 
>> Options:
>> - If there are no slaves yet and the first slave added is tun, we trust
>>   the users to know what they are doing.  Automatically set
>>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
>>   Maybe do a printk to inform the user in case of a mistake.

	I don't think this is feasible, as I don't see a reliable way to
test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
cannot check the ops as they are not statically built into the kernel).
I'm also not sure that heuristics are the proper way to enable this
functionality in general.

>> - If we get an error and the slave device is tun, do a printk giving the
>>   user enough information to find this parameter.

	This could probably be done as a change the existing logic, e.g.,

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1f276fa30ba6..019c1a689aae 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 				res = -EOPNOTSUPP;
 				goto err_undo_flags;
 			}
+		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
+			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
+				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
 		}
 	}
 
	I haven't tested this, and I'm not sure it will get all corner
cases correct, but this should basically cover it.

	-J

>> I'm leaning towards the former, but you probably know a reason why I am
>> wrong again.
>
>Patch below is an implementation of the former.  Not sure if something
>like this is worth considering.
>
>Jörn
>
>--
>To announce that there must be no criticism of the President, or that we
>are to stand by the President, right or wrong, is not only unpatriotic
>and servile, but is morally treasonable to the American public.
>-- Theodore Roosevelt, Kansas City Star, 1918
>
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1f276fa30ba6..306909a44fab 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1482,8 +1482,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	 */
> 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
> 
>-	if (!bond->params.fail_over_mac ||
>-	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>+	if (bond_dev->type != ARPHRD_NONE &&
>+	    (!bond->params.fail_over_mac ||
>+	     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
> 		/* Set slave to master's mac address.  The application already
> 		 * set the master's mac address to that of the first slave
> 		 */
>-- 
>2.1.4
>

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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/11 8:58, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> 
>> On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
>>>
>>> Having to set one more parameter is a bit annoying.  It would have to be
>>> documented in a prominent place and people would still often miss it.
>>> So I wonder if we can make the interface a little nicer.
>>>
>>> Options:
>>> - If there are no slaves yet and the first slave added is tun, we trust
>>>   the users to know what they are doing.  Automatically set
>>>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
>>>   Maybe do a printk to inform the user in case of a mistake.
> 
> 	I don't think this is feasible, as I don't see a reliable way to
> test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
> cannot check the ops as they are not statically built into the kernel).
> I'm also not sure that heuristics are the proper way to enable this
> functionality in general.
> 
>>> - If we get an error and the slave device is tun, do a printk giving the
>>>   user enough information to find this parameter.
> 
> 	This could probably be done as a change the existing logic, e.g.,
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..019c1a689aae 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  				res = -EOPNOTSUPP;
>  				goto err_undo_flags;
>  			}
> +		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
> +				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
>  		}
>  	}
>  
> 	I haven't tested this, and I'm not sure it will get all corner
> cases correct, but this should basically cover it.
> 

Looks fine to cover the case, but if we still let it pass, I am not sure it is suitable.

> 	-J
> 
>>> I'm leaning towards the former, but you probably know a reason why I am
>>> wrong again.
>>
>> Patch below is an implementation of the former.  Not sure if something
>> like this is worth considering.
>>
>> Jörn
>>
>> --
>> To announce that there must be no criticism of the President, or that we
>> are to stand by the President, right or wrong, is not only unpatriotic
>> and servile, but is morally treasonable to the American public.
>> -- Theodore Roosevelt, Kansas City Star, 1918
>>
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..306909a44fab 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1482,8 +1482,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 	 */
>> 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>>
>> -	if (!bond->params.fail_over_mac ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	if (bond_dev->type != ARPHRD_NONE &&
>> +	    (!bond->params.fail_over_mac ||
>> +	     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
>> 		/* Set slave to master's mac address.  The application already
>> 		 * set the master's mac address to that of the first slave
>> 		 */
>> -- 
>> 2.1.4
>>
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> 
> .
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Wed, Aug 10, 2016 at 05:58:38PM -0700, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> >On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
> >> 
> >> Having to set one more parameter is a bit annoying.  It would have to be
> >> documented in a prominent place and people would still often miss it.
> >> So I wonder if we can make the interface a little nicer.
> >> 
> >> Options:
> >> - If there are no slaves yet and the first slave added is tun, we trust
> >>   the users to know what they are doing.  Automatically set
> >>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
> >>   Maybe do a printk to inform the user in case of a mistake.
> 
> 	I don't think this is feasible, as I don't see a reliable way to
> test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
> cannot check the ops as they are not statically built into the kernel).
> I'm also not sure that heuristics are the proper way to enable this
> functionality in general.

I was looking for a slightly more generic thing than "is this device
tun?".  Something along the lines of "is this device L3 only?".  We can
always introduce a new flag and have tun set the flag.  Naïve me thought
ARPHRD_NONE might already match what I was looking for.

But if such an approach causes problems for others, it is a non-starter.

> >> - If we get an error and the slave device is tun, do a printk giving the
> >>   user enough information to find this parameter.
> 
> 	This could probably be done as a change the existing logic, e.g.,
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..019c1a689aae 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  				res = -EOPNOTSUPP;
>  				goto err_undo_flags;
>  			}
> +		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
> +				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
>  		}
>  	}
>  
> 	I haven't tested this, and I'm not sure it will get all corner
> cases correct, but this should basically cover it.

Nit: Indentation is wrong (two tabs instead of one).

It should provide enough information for anyone that reads kernel messages.
Works for me.

[588380.721349] bond1: Adding slave tun0
[588380.721402] bond1: The slave device specified does not support setting the MAC address
[588380.721404] bond1: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

Ping.

Not sure if we reached some conclusion yet.  If we did, I must have
missed it.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/12 2:24, Jörn Engel wrote:
> On Wed, Aug 10, 2016 at 05:58:38PM -0700, Jay Vosburgh wrote:
>> Jörn Engel <joern@purestorage.com> wrote:
>>> On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
>>>>
>>>> Having to set one more parameter is a bit annoying.  It would have to be
>>>> documented in a prominent place and people would still often miss it.
>>>> So I wonder if we can make the interface a little nicer.
>>>>
>>>> Options:
>>>> - If there are no slaves yet and the first slave added is tun, we trust
>>>>   the users to know what they are doing.  Automatically set
>>>>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
>>>>   Maybe do a printk to inform the user in case of a mistake.
>>
>> 	I don't think this is feasible, as I don't see a reliable way to
>> test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
>> cannot check the ops as they are not statically built into the kernel).
>> I'm also not sure that heuristics are the proper way to enable this
>> functionality in general.
> 
> I was looking for a slightly more generic thing than "is this device
> tun?".  Something along the lines of "is this device L3 only?".  We can
> always introduce a new flag and have tun set the flag.  Naïve me thought
> ARPHRD_NONE might already match what I was looking for.
> 

I think there is no such flag to distinguish the tun device, if you insistent on to support new flag
for Tun device, you could send a patch and we could review it, otherwise BOND_FOM_KEEPMAC is enough to
fix this problem.

Thanks
Ding

> But if such an approach causes problems for others, it is a non-starter.
> 
>>>> - If we get an error and the slave device is tun, do a printk giving the
>>>>   user enough information to find this parameter.
>>
>> 	This could probably be done as a change the existing logic, e.g.,
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..019c1a689aae 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>  				res = -EOPNOTSUPP;
>>  				goto err_undo_flags;
>>  			}
>> +		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
>> +			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
>> +				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
>>  		}
>>  	}
>>  
>> 	I haven't tested this, and I'm not sure it will get all corner
>> cases correct, but this should basically cover it.
> 
> Nit: Indentation is wrong (two tabs instead of one).
> 
> It should provide enough information for anyone that reads kernel messages.
> Works for me.
> 
> [588380.721349] bond1: Adding slave tun0
> [588380.721402] bond1: The slave device specified does not support setting the MAC address
> [588380.721404] bond1: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac
> 
> Jörn
> 
> --
> It is easier to lead men to combat, stirring up their passion, than to
> restrain them and direct them toward the patient labours of peace.
> -- Andre Gide
> 
> .
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong]

On 2016/8/9 11:09, Jörn Engel wrote:
> Hello Tianhong!
> 
> On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
>>
>> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
>> and we will try to fix it.
> 
> Here is a trivial testcase:
> openvpn --mktun --dev tun0
> echo +tun0 > /sys/class/net/bond0/bonding/slaves
> 
> Worked fine before your patch, no longer works after your patch.  Works
> again after my patch.
> 
Hi Jorn:

I check the code and know the reason, the Tun device(or something like tun...) didn't has the ndo_set_mac_address, so will not add to bond
as a slaver device in the mode rr, I think the original logic is fine for bond enslave processing, the old kernel just avoid this problem and not
fix it, the bond need to considerate the virtual device which not support changing mac.

>> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
>> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.
> 
> Using bonding in balance-rr mode, there doesn't seem to be a need to
> change the mac address.  I suppose you might care in other modes, but I
> don't.
> 
> Jörn
> 
> --
> Time? What's that? Time is only worth what you do with it.
> -- Theo de Raadt
> 
> 

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jay Vosburgh]

Jörn Engel <joern@purestorage.com> wrote:

>Hello Tianhong!
>
>On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
>> 
>> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
>> and we will try to fix it.
>
>Here is a trivial testcase:
>openvpn --mktun --dev tun0
>echo +tun0 > /sys/class/net/bond0/bonding/slaves
>
>Worked fine before your patch, no longer works after your patch.  Works
>again after my patch.

	Could you describe your use case a bit further?  Are you bonding
together multiple VPN tunnels?

	This may be a regression, but since the patch that nominally
introduced it was 2 years ago, the impact appears to be very narrow.

>> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
>> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.
>
>Using bonding in balance-rr mode, there doesn't seem to be a need to
>change the mac address.  I suppose you might care in other modes, but I
>don't.

	The balance-rr mode (as well as the -xor mode) is designed to
interoperate with a Cisco Etherchannel-style static link aggregation,
which requires all members to have the same MAC address for proper
function.

	Now, the above notwithstanding, I don't have an issue if you
want to bond together a couple of tun devices and can make it work.
However, for the standard balance-rr case, the enslavement must fail if
the call to set the slave MAC fails, as permitting the slave into the
bond after set-MAC fails can result in a bond that silently loses
packets.

	My tentative suggestion is that we extened fail_over_mac to
cover additional modes, as a sort of "I really know what I'm doing"
flag, and allow the enslavement to succeed when it is set.  This would
require setting an additional bonding option for this situation, but
that doesn't seem to be a undue burden as this looks to be a niche use
case.

	-J

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

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Jörn Engel]

On Tue, Aug 09, 2016 at 11:21:31AM -0700, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> >On Tue, Aug 09, 2016 at 10:18:41AM +0800, Ding Tianhong wrote:
> >> 
> >> I don't understand your problem clearly, can you explain more about how the 00503b6f702e break tun-interfaces
> >> and we will try to fix it.
> >
> >Here is a trivial testcase:
> >openvpn --mktun --dev tun0
> >echo +tun0 > /sys/class/net/bond0/bonding/slaves
> >
> >Worked fine before your patch, no longer works after your patch.  Works
> >again after my patch.
> 
> 	Could you describe your use case a bit further?  Are you bonding
> together multiple VPN tunnels?

Yes.  Specificaly I use "ssh -w" to create tunnels.  Ssh is
single-threaded, so the tunnel is too slow.  Aggregate a bunch and you
get closer to link speed.

Alternative would be pfSense.  Afaics that easily beats anything Linux
can offer.  I'm just more familiar with Linux and trust ssh security
more than most alternatives.

> 	This may be a regression, but since the patch that nominally
> introduced it was 2 years ago, the impact appears to be very narrow.

Did you check the dates on the other two bug reports?  Anyone
experiencing the problem and checking google will come to the conclusion
that you don't care and not bother sending yet another bug report.  You
then come to the conclusion that users don't care.

> >> and more, dev_set_mac_address will change the salver's mac address, some nic don't support to change the mac address and
> >> could not work as bond slave, so we need to check the return value, I don't think this patch has any effective improvement.
> >
> >Using bonding in balance-rr mode, there doesn't seem to be a need to
> >change the mac address.  I suppose you might care in other modes, but I
> >don't.
> 
> 	The balance-rr mode (as well as the -xor mode) is designed to
> interoperate with a Cisco Etherchannel-style static link aggregation,
> which requires all members to have the same MAC address for proper
> function.

Linux was designed to be a terminal for dialup to a university in
Helsinki, if memory serves.  Sometimes it is a good thing to work in
ways the design never intended.

Jörn

Re: [PATCH] bonding: Allow tun-interfaces as slaves

[David Miller]

From: Jörn Engel <joern@purestorage.com>
Date: Tue, 9 Aug 2016 11:40:57 -0700

> On Tue, Aug 09, 2016 at 11:21:31AM -0700, Jay Vosburgh wrote:
>> 	The balance-rr mode (as well as the -xor mode) is designed to
>> interoperate with a Cisco Etherchannel-style static link aggregation,
>> which requires all members to have the same MAC address for proper
>> function.
> 
> Linux was designed to be a terminal for dialup to a university in
> Helsinki, if memory serves.  Sometimes it is a good thing to work in
> ways the design never intended.

You're not addressing the issue Jay is trying to make you aware of
in a useful way.

You state that Jay doesn't want to help you, but your comment here
shows that you really aren't exactly participating in the most
positive manner either.