[PATCH] bonding: check if clients MAC addr has changed

[PATCH] bonding: check if clients MAC addr has changed

[Flavio Leitner]

When two systems using bonding devices in adaptive load
balancing (ALB) communicates with each other, an endless
ping-pong of ARP replies starts between these two systems.

What happens? In the ALB mode, bonding driver keeps track
of each client connected in a hash table, so it can do the
receive load balancing (RLB). This hash table is updated
when an ARP reply is received, then it scans for the client
entry, updates its MAC address and flag it to be announced
later. Therefore, two seconds later, the alb monitor runs
and send for each updated client entry two ARP replies
updating this specific client. The same process happens on
the receiving system, causing the endless ping-pong of arp
replies.

See more information including the relevant functions below:

   System 1                          System 2
    bond0                             bond0

   ping <system2>
    ARP request  --------->
                           <--------- ARP reply

+->rlb_arp_recv  <---------------------+   <--- loop begins
|  rlb_update_entry_from_arp           |
|  client_info->ntt = 1;               |
|  bond_info->rx_ntt = 1;              |
|                                      |
|         <communication succeed>      |
|                                      |
|  bond_alb_monitor                    |
|  rlb_update_rx_clients               |
|  rlb_update_client                   |
|  arp_create(ARPOP_REPLY)             |
|   send ARP reply -------------->     V
|   send ARP reply -------------->
|                               rlb_arp_recv
|                               rlb_update_entry_from_arp
|                               client_info->ntt = 1;
|                               bond_info->rx_ntt = 1;
|                           < snipped, same as in system 1>
+-------           <-------------- send ARP reply
                   <-------------- send ARP reply

Besides the unneeded networking traffic, this loop breaks
a cluster because a backup system can't take over the IP
address. There is always one system sending an ARP reply
poisoning the network.

This patch fixes the problem adding a check for the MAC
address before updating it. Thus, if the MAC address didn't
change, there is no need to update neither to announce it later.

Signed-off-by: Flavio Leitner <fleitner@redhat.com>
---
 drivers/net/bonding/bond_alb.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 40fdc41..67154bb 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -340,7 +340,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
 
 	if ((client_info->assigned) &&
 	    (client_info->ip_src == arp->ip_dst) &&
-	    (client_info->ip_dst == arp->ip_src)) {
+	    (client_info->ip_dst == arp->ip_src) &&
+	    (memcmp(client_info->mac_dst, arp->mac_src, ETH_ALEN))) {
 		/* update the clients MAC address */
 		memcpy(client_info->mac_dst, arp->mac_src, ETH_ALEN);
 		client_info->ntt = 1;
-- 
1.7.0.1

Re: [PATCH] bonding: check if clients MAC addr has changed

[Jay Vosburgh]

Flavio Leitner <fleitner@redhat.com> wrote:

>When two systems using bonding devices in adaptive load
>balancing (ALB) communicates with each other, an endless
>ping-pong of ARP replies starts between these two systems.
>
>What happens? In the ALB mode, bonding driver keeps track
>of each client connected in a hash table, so it can do the
>receive load balancing (RLB). This hash table is updated
>when an ARP reply is received, then it scans for the client
>entry, updates its MAC address and flag it to be announced
>later. Therefore, two seconds later, the alb monitor runs
>and send for each updated client entry two ARP replies
>updating this specific client. The same process happens on
>the receiving system, causing the endless ping-pong of arp
>replies.
>
>See more information including the relevant functions below:
>
>   System 1                          System 2
>    bond0                             bond0
>
>   ping <system2>
>    ARP request  --------->
>                           <--------- ARP reply
>
>+->rlb_arp_recv  <---------------------+   <--- loop begins
>|  rlb_update_entry_from_arp           |
>|  client_info->ntt = 1;               |
>|  bond_info->rx_ntt = 1;              |
>|                                      |
>|         <communication succeed>      |
>|                                      |
>|  bond_alb_monitor                    |
>|  rlb_update_rx_clients               |
>|  rlb_update_client                   |
>|  arp_create(ARPOP_REPLY)             |
>|   send ARP reply -------------->     V
>|   send ARP reply -------------->
>|                               rlb_arp_recv
>|                               rlb_update_entry_from_arp
>|                               client_info->ntt = 1;
>|                               bond_info->rx_ntt = 1;
>|                           < snipped, same as in system 1>
>+-------           <-------------- send ARP reply
>                   <-------------- send ARP reply
>
>Besides the unneeded networking traffic, this loop breaks
>a cluster because a backup system can't take over the IP
>address. There is always one system sending an ARP reply
>poisoning the network.
>
>This patch fixes the problem adding a check for the MAC
>address before updating it. Thus, if the MAC address didn't
>change, there is no need to update neither to announce it later.
>
>Signed-off-by: Flavio Leitner <fleitner@redhat.com>
>---
> drivers/net/bonding/bond_alb.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 40fdc41..67154bb 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -340,7 +340,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>
> 	if ((client_info->assigned) &&
> 	    (client_info->ip_src == arp->ip_dst) &&
>-	    (client_info->ip_dst == arp->ip_src)) {
>+	    (client_info->ip_dst == arp->ip_src) &&
>+	    (memcmp(client_info->mac_dst, arp->mac_src, ETH_ALEN))) {

	This should use compare_ether_addr instead of memcmp.

	Other than that, this looks good, so add me to the updated patch:

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

	-J

> 		/* update the clients MAC address */
> 		memcpy(client_info->mac_dst, arp->mac_src, ETH_ALEN);
> 		client_info->ntt = 1;
>-- 
>1.7.0.1

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

Re: [PATCH] bonding: check if clients MAC addr has changed

[Brian Haley]

On 06/29/2010 10:41 AM, Flavio Leitner wrote:
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 40fdc41..67154bb 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -340,7 +340,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>  
>  	if ((client_info->assigned) &&
>  	    (client_info->ip_src == arp->ip_dst) &&
> -	    (client_info->ip_dst == arp->ip_src)) {
> +	    (client_info->ip_dst == arp->ip_src) &&
> +	    (memcmp(client_info->mac_dst, arp->mac_src, ETH_ALEN))) {
>  		/* update the clients MAC address */
>  		memcpy(client_info->mac_dst, arp->mac_src, ETH_ALEN);
>  		client_info->ntt = 1;

compare_ether_addr_64bits() ?

-Brian

Re: [PATCH] bonding: check if clients MAC addr has changed

[Flavio Leitner]

On Tue, Jun 29, 2010 at 11:39:12AM -0400, Brian Haley wrote:
> On 06/29/2010 10:41 AM, Flavio Leitner wrote:
> > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> > index 40fdc41..67154bb 100644
> > --- a/drivers/net/bonding/bond_alb.c
> > +++ b/drivers/net/bonding/bond_alb.c
> > @@ -340,7 +340,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
> >  
> >  	if ((client_info->assigned) &&
> >  	    (client_info->ip_src == arp->ip_dst) &&
> > -	    (client_info->ip_dst == arp->ip_src)) {
> > +	    (client_info->ip_dst == arp->ip_src) &&
> > +	    (memcmp(client_info->mac_dst, arp->mac_src, ETH_ALEN))) {
> >  		/* update the clients MAC address */
> >  		memcpy(client_info->mac_dst, arp->mac_src, ETH_ALEN);
> >  		client_info->ntt = 1;
> 
> compare_ether_addr_64bits() ?

yeah, sounds better. Bonding driver is using it already in
many other places.

-- 
Flavio