Question regarding failure utilizing bonding mode 5 (balance-tlb)

Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Yuval Mintz]

We've had reports that load/unload tests using bonding driver in
balance-tlb mode over bnx2x interfaces results in loss of traffic.

When investigating, we've found out that the bonding driver uses the ndo
(ndo_change_mac_addr()) during ifenslave to override the slaves' HW MAC
address. It then directly goes and changes the slaves netdevices'
dev_addr so that each network interface would posses a distinguish MAC
address (as seen in ifconfig), while the FW/HW of both interfaces is
still configured by the MAC passed by the ndo.

When the active slave is unloaded, the ifconfig MAC (dev_addr) is
swapped between the slaves directly, i.e., without calling the ndo. Once
the interface of the previously active slave will be reloaded, it will
configure it's HW MAC according to that dev_addr value  (i.e., the
bonding driver takes no additional measures to force it's own MAC on the
interface when re-loading), causing it to have a configured MAC which
differs from the one that is held by the bonding driver.

If this is done an additional time (on the newly active slave), both
slave devices will be configured to a MAC which differs from the one
held by the bond interface (i.e., the bond interface holds the MAC of
the original active slave, while both interfaces configured the MAC of
the original inactive slave). This obviously prevents any traffic from
being successfully sent/received.

bnx2x uses dev_addr directly for MAC configuration, which I think is the
default behaviour for most network drivers - ixgbe has a shadow value
which it uses instead, but I think that's the exception and not the
rule.

As I see it, either:

   1. The bonding driver is flawed in balance-tlb mode and should be
fixed.

   2. bnx2x's behaviour is flawed - it should have some persistent
shadow MAC which should contain the last MAC set - either factory value
or what was configured by the ndo, and use it instead of dev_addr when
configuring the HW MAC.
This would probably indicate that other drivers are flawed as well.

   3. The test itself is flawed, since user should not unload slave
interfaces.

What's the correct approach for fixing the issue?
Idea's will be welcomed.

Thanks,
Yuval

Re: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Jay Vosburgh]

Yuval Mintz <yuvalmin@broadcom.com> wrote:

>We've had reports that load/unload tests using bonding driver in
>balance-tlb mode over bnx2x interfaces results in loss of traffic.

	I've also been looking into what I suspect is the same thing,
although using bnx2 and not bnx2x.

>When investigating, we've found out that the bonding driver uses the ndo
>(ndo_change_mac_addr()) during ifenslave to override the slaves' HW MAC
>address. It then directly goes and changes the slaves netdevices'
>dev_addr so that each network interface would posses a distinguish MAC
>address (as seen in ifconfig), while the FW/HW of both interfaces is
>still configured by the MAC passed by the ndo.

	Yes.

>When the active slave is unloaded, the ifconfig MAC (dev_addr) is
>swapped between the slaves directly, i.e., without calling the ndo. Once
>the interface of the previously active slave will be reloaded, it will
>configure it's HW MAC according to that dev_addr value  (i.e., the
>bonding driver takes no additional measures to force it's own MAC on the
>interface when re-loading), causing it to have a configured MAC which
>differs from the one that is held by the bonding driver.

	I'm not sure I follow this part, looking at the code.
Basically, and correct me if I'm missing something, what you're
describing is this:

	1. Add and remove some slaves until the removed slave ends up
with dev_addr set to something stale (not it's nominal permanent
hardware address).

	2. Enslave that device, bond_enslave calls dev_open, and the
driver's open function programs the device's MAC to what's in dev_addr

	The part I don't follow is that in bond_enslave, this sequence
occurs:

	1. bond_enslave calls dev_set_mac_address ("the ndo") to program
the newly added slave with the master's MAC.  The ndo_set_mac_address
functions for bnx2x and bnx2 both set dev_addr to the new address.

	2. bond_enslave calls dev_open, and the driver's open function
programs the device's MAC to what's in dev_addr, which is now the
master's MAC address.

	The above is true, unless fail_over_mac is enabled, and that's
not a valid option for tlb mode.  

	Also, in theory the bond will reset the slave's MAC address to
its "permanent" address when it is released from the bond.  The
"permanent" address is whatever was in dev_addr when the device was
enslaved.

	Am I misunderstanding something here?

>If this is done an additional time (on the newly active slave), both
>slave devices will be configured to a MAC which differs from the one
>held by the bond interface (i.e., the bond interface holds the MAC of
>the original active slave, while both interfaces configured the MAC of
>the original inactive slave). This obviously prevents any traffic from
>being successfully sent/received.

	Now, this part does explain the end result that we see as well,
although it's been more random here (we did not have a specific recipe
to induce it, so I'll be trying out yours as soon as I can).  The device
can TX just fine, but all incoming traffic is dropped.  Placing the
device into promiscuous mode works around the problem for as long as
promisc is enabled.

>bnx2x uses dev_addr directly for MAC configuration, which I think is the
>default behaviour for most network drivers - ixgbe has a shadow value
>which it uses instead, but I think that's the exception and not the
>rule.
>
>As I see it, either:
>
>   1. The bonding driver is flawed in balance-tlb mode and should be
>fixed.
>
>   2. bnx2x's behaviour is flawed - it should have some persistent
>shadow MAC which should contain the last MAC set - either factory value
>or what was configured by the ndo, and use it instead of dev_addr when
>configuring the HW MAC.
>This would probably indicate that other drivers are flawed as well.
>
>   3. The test itself is flawed, since user should not unload slave
>interfaces.
>
>What's the correct approach for fixing the issue?

	Well, I suspect it's not going to be #2.  Loading and unloading
slaves ought to work, and I'm willing to believe that bonding is doing
something odd, but I don't see what it is from the above.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

RE: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Yuval Mintz]

> >We've had reports that load/unload tests using bonding driver in
> >balance-tlb mode over bnx2x interfaces results in loss of traffic.
> 
> 	I've also been looking into what I suspect is the same thing,
> although using bnx2 and not bnx2x.

Makes sense, given that both follow the same paradigms.

> >When the active slave is unloaded, the ifconfig MAC (dev_addr) is
> >swapped between the slaves directly, i.e., without calling the ndo. Once
> >the interface of the previously active slave will be reloaded, it will
> >configure it's HW MAC according to that dev_addr value  (i.e., the
> >bonding driver takes no additional measures to force it's own MAC on the
> >interface when re-loading), causing it to have a configured MAC which
> >differs from the one that is held by the bonding driver.
> 	The part I don't follow is that in bond_enslave, this sequence
> occurs:
> 
> 	1. bond_enslave calls dev_set_mac_address ("the ndo") to program
> the newly added slave with the master's MAC.  The ndo_set_mac_address
> functions for bnx2x and bnx2 both set dev_addr to the new address.
> 
> 	2. bond_enslave calls dev_open, and the driver's open function
> programs the device's MAC to what's in dev_addr, which is now the
> master's MAC address.

I think 'bond_enslave' is called only on initial enslavement - the code 
doesn't  make sense for me otherwise (as it seems the IFF_SLAVE indication
will be removed only when the slave notify of NETDEV_UNREGISTER, i.e., 
when it is rmmoded and not the interface is closed). 

> 
> 	The above is true, unless fail_over_mac is enabled, and that's
> not a valid option for tlb mode.
> 
> 	Also, in theory the bond will reset the slave's MAC address to
> its "permanent" address when it is released from the bond.  The
> "permanent" address is whatever was in dev_addr when the device was
> enslaved.

Again, I think the permanent address is restored only when the bond
releases the slave, which I don't think happens when the slave is unloaded. 

> >As I see it, either:
> >
> >   1. The bonding driver is flawed in balance-tlb mode and should be
> >fixed.
> >
> >   2. bnx2x's behaviour is flawed - it should have some persistent
> >shadow MAC which should contain the last MAC set - either factory value
> >or what was configured by the ndo, and use it instead of dev_addr when
> >configuring the HW MAC.
> >This would probably indicate that other drivers are flawed as well.
> >
> >   3. The test itself is flawed, since user should not unload slave
> >interfaces.
> >
> >What's the correct approach for fixing the issue?
> 
> 	Well, I suspect it's not going to be #2.  Loading and unloading
> slaves ought to work, and I'm willing to believe that bonding is doing
> something odd, but I don't see what it is from the above.
> 
> 	-J
> 

Re: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Jay Vosburgh]

Yuval Mintz <yuvalmin@broadcom.com> wrote:

>> >We've had reports that load/unload tests using bonding driver in
>> >balance-tlb mode over bnx2x interfaces results in loss of traffic.
>> 
>> 	I've also been looking into what I suspect is the same thing,
>> although using bnx2 and not bnx2x.
>
>Makes sense, given that both follow the same paradigms.
>
>> >When the active slave is unloaded, the ifconfig MAC (dev_addr) is
>> >swapped between the slaves directly, i.e., without calling the ndo. Once
>> >the interface of the previously active slave will be reloaded, it will
>> >configure it's HW MAC according to that dev_addr value  (i.e., the
>> >bonding driver takes no additional measures to force it's own MAC on the
>> >interface when re-loading), causing it to have a configured MAC which
>> >differs from the one that is held by the bonding driver.
>> 	The part I don't follow is that in bond_enslave, this sequence
>> occurs:
>> 
>> 	1. bond_enslave calls dev_set_mac_address ("the ndo") to program
>> the newly added slave with the master's MAC.  The ndo_set_mac_address
>> functions for bnx2x and bnx2 both set dev_addr to the new address.
>> 
>> 	2. bond_enslave calls dev_open, and the driver's open function
>> programs the device's MAC to what's in dev_addr, which is now the
>> master's MAC address.
>
>I think 'bond_enslave' is called only on initial enslavement - the code 
>doesn't  make sense for me otherwise (as it seems the IFF_SLAVE indication
>will be removed only when the slave notify of NETDEV_UNREGISTER, i.e., 
>when it is rmmoded and not the interface is closed). 
>> 
>> 	The above is true, unless fail_over_mac is enabled, and that's
>> not a valid option for tlb mode.
>> 
>> 	Also, in theory the bond will reset the slave's MAC address to
>> its "permanent" address when it is released from the bond.  The
>> "permanent" address is whatever was in dev_addr when the device was
>> enslaved.
>
>Again, I think the permanent address is restored only when the bond
>releases the slave, which I don't think happens when the slave is unloaded. 

	Ah, ok, I was understanding "unloaded" to mean "remove from the
bond."  I think you actually mean "set administratively down," e.g., "ip
link set dev slave down" or the like.  I don't think mere loss of
carrier would trigger the sequence of events, because that won't go
through a dev_close / dev_open cycle.

	Doing that (an admin down / up bounce) would, indeed, cause a
failover, but the bond will not reprogram the MAC on the slave (it
presumes that a fail / recovery will not disrupt the MAC address, which
is apparently not true in this instance).

	I'll have to look at the code a bit, but for now can you confirm
that what you actually mean is, essentially:

	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
being the active,

	1) "ip link set dev eth0 down" which will fail over to eth1
		(swapping the contents of their dev_addr fields).

	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
		MAC to the wrong thing (what was in dev_addr).

	3) repeat steps 1 and 2 for eth1

	Is this correct?

>> >As I see it, either:
>> >
>> >   1. The bonding driver is flawed in balance-tlb mode and should be
>> >fixed.
>> >
>> >   2. bnx2x's behaviour is flawed - it should have some persistent
>> >shadow MAC which should contain the last MAC set - either factory value
>> >or what was configured by the ndo, and use it instead of dev_addr when
>> >configuring the HW MAC.
>> >This would probably indicate that other drivers are flawed as well.
>> >
>> >   3. The test itself is flawed, since user should not unload slave
>> >interfaces.
>> >
>> >What's the correct approach for fixing the issue?
>> 
>> 	Well, I suspect it's not going to be #2.  Loading and unloading
>> slaves ought to work, and I'm willing to believe that bonding is doing
>> something odd, but I don't see what it is from the above.

	I think my above statement is still true, but fixing this may be
a bit trickier, or may be a "best effort" type of thing.  One reason is
that, nominally, the tlb mode does not require that the device be able
to change (meaning the ndo call to reprogram) its MAC while open.

	This may not really be a meaningful restriction now, but when
the code was written, not every device / driver could change MAC while
open.  I'm unsure if there are current users that rely on this.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

RE: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Yuval Mintz]

> >Again, I think the permanent address is restored only when the bond
> >releases the slave, which I don't think happens when the slave is unloaded.
> 
> 	Ah, ok, I was understanding "unloaded" to mean "remove from the
> bond."  I think you actually mean "set administratively down," e.g., "ip
> link set dev slave down" or the like.  I don't think mere loss of
> carrier would trigger the sequence of events, because that won't go
> through a dev_close / dev_open cycle.
> 
> 	Doing that (an admin down / up bounce) would, indeed, cause a
> failover, but the bond will not reprogram the MAC on the slave (it
> presumes that a fail / recovery will not disrupt the MAC address, which
> is apparently not true in this instance).
> 
> 	I'll have to look at the code a bit, but for now can you confirm
> that what you actually mean is, essentially:
> 
> 	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
> being the active,
> 
> 	1) "ip link set dev eth0 down" which will fail over to eth1
> 		(swapping the contents of their dev_addr fields).
> 
> 	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
> 		MAC to the wrong thing (what was in dev_addr).
> 
> 	3) repeat steps 1 and 2 for eth1
> 
> 	Is this correct?
> 

Yes, sorry for the earlier confusion.
I think in the case described `alb_swap_mac_addr()' will be called, 
replacing eth0 and eth1's dev_addr, causing eth0 to have dev_addr
which defers from  the bond device's. Once eth0 reloads, it will use
the different MAC address for configuring FW/HW.

RE: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Yuval Mintz]

> > >Again, I think the permanent address is restored only when the bond
> > >releases the slave, which I don't think happens when the slave is unloaded.
> >
> > 	Ah, ok, I was understanding "unloaded" to mean "remove from the
> > bond."  I think you actually mean "set administratively down," e.g., "ip
> > link set dev slave down" or the like.  I don't think mere loss of
> > carrier would trigger the sequence of events, because that won't go
> > through a dev_close / dev_open cycle.
> >
> > 	Doing that (an admin down / up bounce) would, indeed, cause a
> > failover, but the bond will not reprogram the MAC on the slave (it
> > presumes that a fail / recovery will not disrupt the MAC address, which
> > is apparently not true in this instance).
> >
> > 	I'll have to look at the code a bit, but for now can you confirm
> > that what you actually mean is, essentially:
> >
> > 	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
> > being the active,
> >
> > 	1) "ip link set dev eth0 down" which will fail over to eth1
> > 		(swapping the contents of their dev_addr fields).
> >
> > 	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
> > 		MAC to the wrong thing (what was in dev_addr).
> >
> > 	3) repeat steps 1 and 2 for eth1
> >
> > 	Is this correct?
> >
> 
> Yes, sorry for the earlier confusion.
> I think in the case described `alb_swap_mac_addr()' will be called,
> replacing eth0 and eth1's dev_addr, causing eth0 to have dev_addr
> which defers from  the bond device's. Once eth0 reloads, it will use
> the different MAC address for configuring FW/HW.

Hi,

Did you by any chance had the time to look at this issue?

Thanks,
Yuval

Re: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Veaceslav Falico]

On Mon, Sep 30, 2013 at 11:30:40AM +0000, Yuval Mintz wrote:
>> > >Again, I think the permanent address is restored only when the bond
>> > >releases the slave, which I don't think happens when the slave is unloaded.
>> >
>> > 	Ah, ok, I was understanding "unloaded" to mean "remove from the
>> > bond."  I think you actually mean "set administratively down," e.g., "ip
>> > link set dev slave down" or the like.  I don't think mere loss of
>> > carrier would trigger the sequence of events, because that won't go
>> > through a dev_close / dev_open cycle.
>> >
>> > 	Doing that (an admin down / up bounce) would, indeed, cause a
>> > failover, but the bond will not reprogram the MAC on the slave (it
>> > presumes that a fail / recovery will not disrupt the MAC address, which
>> > is apparently not true in this instance).
>> >
>> > 	I'll have to look at the code a bit, but for now can you confirm
>> > that what you actually mean is, essentially:
>> >
>> > 	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
>> > being the active,
>> >
>> > 	1) "ip link set dev eth0 down" which will fail over to eth1
>> > 		(swapping the contents of their dev_addr fields).
>> >
>> > 	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
>> > 		MAC to the wrong thing (what was in dev_addr).
>> >
>> > 	3) repeat steps 1 and 2 for eth1
>> >
>> > 	Is this correct?
>> >
>>
>> Yes, sorry for the earlier confusion.
>> I think in the case described `alb_swap_mac_addr()' will be called,
>> replacing eth0 and eth1's dev_addr, causing eth0 to have dev_addr
>> which defers from  the bond device's. Once eth0 reloads, it will use
>> the different MAC address for configuring FW/HW.
>
>Hi,
>
>Did you by any chance had the time to look at this issue?

Hi Yuval,

Sorry for getting into the discussion - but I've tried to understand the
problem and, possibly, find a fix.

I'm not sure that I completely understand it, and I don't have currently
hardware on which to test it (though I might have it in the nearest
future), so, again, I really am not sure that I won't suggest something
completely stupid.

Anyway, that being said, I hope that the following patch might fix the
problem. I've described the bug and the fix in the changelog, and the code
is pretty self-explanitory.

And even if the patch fixes the issue - I'm not sure that it's the proper
and correct way to do it. But it's definitely worth a try... So, if it's
possible, could you please test this patch and see if it fixes it?

Warning: I've just compile-tested it.

So, FWIW...

 From 87e6c584b0ae0f0261610d60cf83778feb9c1edb Mon Sep 17 00:00:00 2001
From: Veaceslav Falico <vfalico@redhat.com>
Date: Mon, 30 Sep 2013 23:14:23 +0200
Subject: [PATCH] bonding: ensure that TLB mode's active slave has correct mac filter

Currently, in TLB mode we change mac addresses only by memcpy-ing the to
net_device->dev_addr, without actually setting them via
dev_set_mac_address(). This permits us to receive all the traffic always on
one mac address.

However, in case the interface flips, some drivers might enforce the
mac filtering for its FW/HW based on current ->dev_addr, and thus we won't
be able to receive traffic on that interface, in case it will be selected
as active in TLB mode.

Fix it by setting the mac address forcefully on every new active slave that
we select in TLB mode.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
  drivers/net/bonding/bond_alb.c | 17 +++++++++++++++++
  1 file changed, 17 insertions(+)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index e960418..576ccea 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1699,6 +1699,23 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
  
  	ASSERT_RTNL();
  
+	/* in TLB mode, the slave might flip down/up with the old dev_addr,
+	 * and thus filter bond->dev_addr's packets, so force bond's mac
+	 */
+	if (bond->params.mode == BOND_MODE_TLB) {
+		struct sockaddr sa;
+		u8 tmp_addr[ETH_ALEN];
+
+		memcpy(tmp_addr, new_slave->dev->dev_addr, ETH_ALEN);
+
+		memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len);
+		sa.sa_family = bond->dev->type;
+		/* we don't care if it can't change its mac, best effort */
+		dev_set_mac_address(new_slave->dev, &sa);
+
+		memcpy(new_slave->dev->dev_addr, tmp_addr, ETH_ALEN);
+	}
+
  	/* curr_active_slave must be set before calling alb_swap_mac_addr */
  	if (swap_slave) {
  		/* swap mac address */
-- 
1.8.4


>
>Thanks,
>Yuval
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Yuval Mintz]

> On Mon, Sep 30, 2013 at 11:30:40AM +0000, Yuval Mintz wrote:
> >> > >Again, I think the permanent address is restored only when the bond
> >> > >releases the slave, which I don't think happens when the slave is
> unloaded.
> >> >
> >> > 	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
> >> > being the active,
> >> >
> >> > 	1) "ip link set dev eth0 down" which will fail over to eth1
> >> > 		(swapping the contents of their dev_addr fields).
> >> >
> >> > 	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
> >> > 		MAC to the wrong thing (what was in dev_addr).
> >> >
> >> > 	3) repeat steps 1 and 2 for eth1
> >> >
> >> > 	Is this correct?
> >> >
> >>
> >> Yes, sorry for the earlier confusion.
> >> I think in the case described `alb_swap_mac_addr()' will be called,
> >> replacing eth0 and eth1's dev_addr, causing eth0 to have dev_addr
> >> which defers from  the bond device's. Once eth0 reloads, it will use
> >> the different MAC address for configuring FW/HW.
> >
> >Hi,
> >
> >Did you by any chance had the time to look at this issue?
> 
> Hi Yuval,
> 
> Sorry for getting into the discussion - but I've tried to understand the
> problem and, possibly, find a fix.
> 
> I'm not sure that I completely understand it, and I don't have currently
> hardware on which to test it (though I might have it in the nearest
> future), so, again, I really am not sure that I won't suggest something
> completely stupid.
> 
> Anyway, that being said, I hope that the following patch might fix the
> problem. I've described the bug and the fix in the changelog, and the code
> is pretty self-explanitory.
> 
> And even if the patch fixes the issue - I'm not sure that it's the proper
> and correct way to do it. But it's definitely worth a try... So, if it's
> possible, could you please test this patch and see if it fixes it?
> 
> Warning: I've just compile-tested it.
> 
> So, FWIW...
> 

Like you, I don't know if yours is the proper way of fixing the issue - but it did
seem to fix it (the scenario that was described, at least)

Tested-by: Yuval Mintz <yuvalmin@broadcom.com>

Thanks,
Yuval

>  From 87e6c584b0ae0f0261610d60cf83778feb9c1edb Mon Sep 17
> 00:00:00 2001
> From: Veaceslav Falico <vfalico@redhat.com>
> Date: Mon, 30 Sep 2013 23:14:23 +0200
> Subject: [PATCH] bonding: ensure that TLB mode's active slave has correct
> mac filter
> 
> Currently, in TLB mode we change mac addresses only by memcpy-ing the to
> net_device->dev_addr, without actually setting them via
> dev_set_mac_address(). This permits us to receive all the traffic always on
> one mac address.
> 
> However, in case the interface flips, some drivers might enforce the
> mac filtering for its FW/HW based on current ->dev_addr, and thus we won't
> be able to receive traffic on that interface, in case it will be selected
> as active in TLB mode.
> 
> Fix it by setting the mac address forcefully on every new active slave that
> we select in TLB mode.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>   drivers/net/bonding/bond_alb.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index e960418..576ccea 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -1699,6 +1699,23 @@ void bond_alb_handle_active_change(struct
> bonding *bond, struct slave *new_slave
> 
>   	ASSERT_RTNL();
> 
> +	/* in TLB mode, the slave might flip down/up with the old dev_addr,
> +	 * and thus filter bond->dev_addr's packets, so force bond's mac
> +	 */
> +	if (bond->params.mode == BOND_MODE_TLB) {
> +		struct sockaddr sa;
> +		u8 tmp_addr[ETH_ALEN];
> +
> +		memcpy(tmp_addr, new_slave->dev->dev_addr, ETH_ALEN);
> +
> +		memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev-
> >addr_len);
> +		sa.sa_family = bond->dev->type;
> +		/* we don't care if it can't change its mac, best effort */
> +		dev_set_mac_address(new_slave->dev, &sa);
> +
> +		memcpy(new_slave->dev->dev_addr, tmp_addr, ETH_ALEN);
> +	}
> +
>   	/* curr_active_slave must be set before calling alb_swap_mac_addr
> */
>   	if (swap_slave) {
>   		/* swap mac address */
> --
> 1.8.4
> 
> 
> >
> >Thanks,
> >Yuval
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding failure utilizing bonding mode 5 (balance-tlb)

[Veaceslav Falico]

On Tue, Oct 01, 2013 at 12:56:43PM +0000, Yuval Mintz wrote:
>> On Mon, Sep 30, 2013 at 11:30:40AM +0000, Yuval Mintz wrote:
>> >> > >Again, I think the permanent address is restored only when the bond
>> >> > >releases the slave, which I don't think happens when the slave is
>> unloaded.
>> >> >
>> >> > 	Given a bond0 with two slaves, eth0 and eth1, in tlb mode, eth0
>> >> > being the active,
>> >> >
>> >> > 	1) "ip link set dev eth0 down" which will fail over to eth1
>> >> > 		(swapping the contents of their dev_addr fields).
>> >> >
>> >> > 	2) "ip link set dev eth0 up" eth0 comes back up, reprograms its
>> >> > 		MAC to the wrong thing (what was in dev_addr).
>> >> >
>> >> > 	3) repeat steps 1 and 2 for eth1
>> >> >
>> >> > 	Is this correct?
>> >> >
>> >>
>> >> Yes, sorry for the earlier confusion.
>> >> I think in the case described `alb_swap_mac_addr()' will be called,
>> >> replacing eth0 and eth1's dev_addr, causing eth0 to have dev_addr
>> >> which defers from  the bond device's. Once eth0 reloads, it will use
>> >> the different MAC address for configuring FW/HW.
>> >
>> >Hi,
>> >
>> >Did you by any chance had the time to look at this issue?
>>
>> Hi Yuval,
>>
>> Sorry for getting into the discussion - but I've tried to understand the
>> problem and, possibly, find a fix.
>>
>> I'm not sure that I completely understand it, and I don't have currently
>> hardware on which to test it (though I might have it in the nearest
>> future), so, again, I really am not sure that I won't suggest something
>> completely stupid.
>>
>> Anyway, that being said, I hope that the following patch might fix the
>> problem. I've described the bug and the fix in the changelog, and the code
>> is pretty self-explanitory.
>>
>> And even if the patch fixes the issue - I'm not sure that it's the proper
>> and correct way to do it. But it's definitely worth a try... So, if it's
>> possible, could you please test this patch and see if it fixes it?
>>
>> Warning: I've just compile-tested it.
>>
>> So, FWIW...
>>
>
>Like you, I don't know if yours is the proper way of fixing the issue - but it did
>seem to fix it (the scenario that was described, at least)
>
>Tested-by: Yuval Mintz <yuvalmin@broadcom.com>

Thank you!

I'll then try now to dig it a big futher, and if it happens that this fix is
really the one we need - I'll use your Tested-by, hope that you're ok with
that. Otherwise I'll send a new patch(set) with you in the CC.

Thanks again!

>
>Thanks,
>Yuval
>
>>  From 87e6c584b0ae0f0261610d60cf83778feb9c1edb Mon Sep 17
>> 00:00:00 2001
>> From: Veaceslav Falico <vfalico@redhat.com>
>> Date: Mon, 30 Sep 2013 23:14:23 +0200
>> Subject: [PATCH] bonding: ensure that TLB mode's active slave has correct
>> mac filter
>>
>> Currently, in TLB mode we change mac addresses only by memcpy-ing the to
>> net_device->dev_addr, without actually setting them via
>> dev_set_mac_address(). This permits us to receive all the traffic always on
>> one mac address.
>>
>> However, in case the interface flips, some drivers might enforce the
>> mac filtering for its FW/HW based on current ->dev_addr, and thus we won't
>> be able to receive traffic on that interface, in case it will be selected
>> as active in TLB mode.
>>
>> Fix it by setting the mac address forcefully on every new active slave that
>> we select in TLB mode.
>>
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>   drivers/net/bonding/bond_alb.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index e960418..576ccea 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -1699,6 +1699,23 @@ void bond_alb_handle_active_change(struct
>> bonding *bond, struct slave *new_slave
>>
>>   	ASSERT_RTNL();
>>
>> +	/* in TLB mode, the slave might flip down/up with the old dev_addr,
>> +	 * and thus filter bond->dev_addr's packets, so force bond's mac
>> +	 */
>> +	if (bond->params.mode == BOND_MODE_TLB) {
>> +		struct sockaddr sa;
>> +		u8 tmp_addr[ETH_ALEN];
>> +
>> +		memcpy(tmp_addr, new_slave->dev->dev_addr, ETH_ALEN);
>> +
>> +		memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev-
>> >addr_len);
>> +		sa.sa_family = bond->dev->type;
>> +		/* we don't care if it can't change its mac, best effort */
>> +		dev_set_mac_address(new_slave->dev, &sa);
>> +
>> +		memcpy(new_slave->dev->dev_addr, tmp_addr, ETH_ALEN);
>> +	}
>> +
>>   	/* curr_active_slave must be set before calling alb_swap_mac_addr
>> */
>>   	if (swap_slave) {
>>   		/* swap mac address */
>> --
>> 1.8.4
>>
>>
>> >
>> >Thanks,
>> >Yuval
>> >
>> >--
>> >To unsubscribe from this list: send the line "unsubscribe netdev" in
>> >the body of a message to majordomo@vger.kernel.org
>> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>