bonding: set device in RLB ARP packet handler

PATCH New Greg Edwards 4 participants 4 messages Jul 22, 2010 – Jul 25, 2010

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Greg Edwards

author

2010-07-22 19:52 UTC #0 lore raw

PATCH 1/1 bonding: set device in RLB ARP packet handler

With commit 6146b1a4, the dev field in the RLB ARP packet handler was set to NULL to wildcard and accommodate balancing VLANs on top of bonds.

This has the side-effect of the packet handler being called against other, non RLB-enabled bonds, and a kernel oops results when it tries to dereference rx_hashtbl in rlb_update_entry_from_arp(), which won't be set for those bonds, e.g. active-backup.

With the __netif_receive_skb() changes from commit 1f3c8804, frames received on VLANs correctly make their way to the bond's handler, so we no longer need to wildcard the device.

Signed-off-by Greg Edwards <[email protected]>

---
Jay,

The oops can be reproduced by:

modprobe bonding

echo active-backup > /sys/class/net/bond0/bonding/mode echo 100 > /sys/class/net/bond0/bonding/miimon ifconfig bond0 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth0 > /sys/class/net/bond0/bonding/slaves echo +eth1 > /sys/class/net/bond0/bonding/slaves

echo +bond1 > /sys/class/net/bonding_masters echo balance-alb > /sys/class/net/bond1/bonding/mode echo 100 > /sys/class/net/bond1/bonding/miimon ifconfig bond1 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth2 > /sys/class/net/bond1/bonding/slaves echo +eth3 > /sys/class/net/bond1/bonding/slaves

Pass some traffic on bond0. Boom.

 drivers/net/bonding/bond_alb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/bonding/bond_alb.c +1 -1

index df48307..8d7dfd2 100644
@@ -822,7 +822,7 @@ static int rlb_initialize(struct bonding *bond)
 
 	/*initialize packet type*/
 	pk_type->type = cpu_to_be16(ETH_P_ARP);
-	pk_type->dev = NULL;
+	pk_type->dev = bond->dev;
 	pk_type->func = rlb_arp_recv;
 
 	/* register to receive ARPs */

Signature · 1 line

1.7.0.4

Jay Vosburgh

2010-07-23 20:02 UTC #1 lore raw

PATCH 1/1 bonding: set device in RLB ARP packet handler

From: Greg Edwards <[email protected]>

After:

commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e

Author: Jay Vosburgh <[email protected]>
Date:   Tue Nov 4 17:51:15 2008 -0800

bonding: Fix ALB mode to balance traffic on VLANs

the dev field in the RLB ARP packet handler was set to NULL to wildcard and accommodate balancing VLANs on top of bonds.

With the __netif_receive_skb() changes from:

commit 1f3c8804acba841b5573b953f5560d2683d2db0d

Author: Andy Gospodarek <[email protected]>
Date:   Mon Dec 14 10:48:58 2009 +0000

bonding: allow arp_ip_targets on separate vlans to use arp validation

frames received on VLANs correctly make their way to the bond's handler, so we no longer need to wildcard the device.

The oops can be reproduced by:

modprobe bonding

Pass some traffic on bond0. Boom.

[ Tested, behaves as advertised. I do not believe a test of the bonding mode is necessary, as there is no race between the packet handler and the bonding mode changing (the mode can only change when the device is closed). Also updated the log message to include the reproduction and full commit ids. -J ]

Signed-off-by Greg Edwards <[email protected]>

Signed-off-by Jay Vosburgh <[email protected]>

---

 drivers/net/bonding/bond_alb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/bonding/bond_alb.c +1 -1

index df48307..8d7dfd2 100644
@@ -822,7 +822,7 @@ static int rlb_initialize(struct bonding *bond)
 
 	/*initialize packet type*/
 	pk_type->type = cpu_to_be16(ETH_P_ARP);
-	pk_type->dev = NULL;
+	pk_type->dev = bond->dev;
 	pk_type->func = rlb_arp_recv;
 
 	/* register to receive ARPs */

Signature · 1 line

1.7.0.4

Andy Gospodarek

Acked-by on v1 1/1

2010-07-23 20:26 UTC #2 lore raw

Quoted 49 lines · Jay Vosburgh

From: Greg Edwards <[email protected]>
After:
commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e
Author: Jay Vosburgh <[email protected]>
Date:   Tue Nov 4 17:51:15 2008 -0800
bonding: Fix ALB mode to balance traffic on VLANs
the dev field in the RLB ARP packet handler was set to NULL to wildcard and accommodate balancing VLANs on top of bonds.
This has the side-effect of the packet handler being called against other, non RLB-enabled bonds, and a kernel oops results when it tries to dereference rx_hashtbl in rlb_update_entry_from_arp(), which won't be set for those bonds, e.g. active-backup.
With the __netif_receive_skb() changes from:
commit 1f3c8804acba841b5573b953f5560d2683d2db0d
Author: Andy Gospodarek <[email protected]>
Date:   Mon Dec 14 10:48:58 2009 +0000
bonding: allow arp_ip_targets on separate vlans to use arp validation
frames received on VLANs correctly make their way to the bond's handler, so we no longer need to wildcard the device.
The oops can be reproduced by:
modprobe bonding
echo active-backup > /sys/class/net/bond0/bonding/mode echo 100 > /sys/class/net/bond0/bonding/miimon ifconfig bond0 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth0 > /sys/class/net/bond0/bonding/slaves echo +eth1 > /sys/class/net/bond0/bonding/slaves
echo +bond1 > /sys/class/net/bonding_masters echo balance-alb > /sys/class/net/bond1/bonding/mode echo 100 > /sys/class/net/bond1/bonding/miimon ifconfig bond1 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth2 > /sys/class/net/bond1/bonding/slaves echo +eth3 > /sys/class/net/bond1/bonding/slaves
Pass some traffic on bond0. Boom.

Quoted 8 lines

[ Tested, behaves as advertised. I do not believe a test of the bonding mode is necessary, as there is no race between the packet handler and the bonding mode changing (the mode can only change when the device is closed). Also updated the log message to include the reproduction and full commit ids. -J ]
Signed-off-by Greg Edwards <[email protected]>
Signed-off-by Jay Vosburgh <[email protected]>

Acked-by Andy Gospodarek <[email protected]>

Quoted 26 lines

---

 drivers/net/bonding/bond_alb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/bonding/bond_alb.c +1 -1

index df48307..8d7dfd2 100644
@@ -822,7 +822,7 @@ static int rlb_initialize(struct bonding *bond)
 
 	/*initialize packet type*/
 	pk_type->type = cpu_to_be16(ETH_P_ARP);
-	pk_type->dev = NULL;
+	pk_type->dev = bond->dev;
 	pk_type->func = rlb_arp_recv;
 
 	/* register to receive ARPs */

Signature · 6 lines

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2010-07-25 03:38 UTC #3 lore raw

From: Andy Gospodarek <[email protected]>
Date: Fri, 23 Jul 2010 16:26:48 -0400

Quoted 56 lines · Jay Vosburgh

Quoted 55 lines
From: Greg Edwards <[email protected]>
After:
commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e
Author: Jay Vosburgh <[email protected]>
Date:   Tue Nov 4 17:51:15 2008 -0800
bonding: Fix ALB mode to balance traffic on VLANs
the dev field in the RLB ARP packet handler was set to NULL to wildcard and accommodate balancing VLANs on top of bonds.
This has the side-effect of the packet handler being called against other, non RLB-enabled bonds, and a kernel oops results when it tries to dereference rx_hashtbl in rlb_update_entry_from_arp(), which won't be set for those bonds, e.g. active-backup.
With the __netif_receive_skb() changes from:
commit 1f3c8804acba841b5573b953f5560d2683d2db0d
Author: Andy Gospodarek <[email protected]>
Date:   Mon Dec 14 10:48:58 2009 +0000
bonding: allow arp_ip_targets on separate vlans to use arp validation
frames received on VLANs correctly make their way to the bond's handler, so we no longer need to wildcard the device.
The oops can be reproduced by:
modprobe bonding
echo active-backup > /sys/class/net/bond0/bonding/mode echo 100 > /sys/class/net/bond0/bonding/miimon ifconfig bond0 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth0 > /sys/class/net/bond0/bonding/slaves echo +eth1 > /sys/class/net/bond0/bonding/slaves
echo +bond1 > /sys/class/net/bonding_masters echo balance-alb > /sys/class/net/bond1/bonding/mode echo 100 > /sys/class/net/bond1/bonding/miimon ifconfig bond1 xxx.xxx.xxx.xxx netmask xxx.xxx.xxx.xxx echo +eth2 > /sys/class/net/bond1/bonding/slaves echo +eth3 > /sys/class/net/bond1/bonding/slaves
Pass some traffic on bond0. Boom.
[ Tested, behaves as advertised. I do not believe a test of the bonding mode is necessary, as there is no race between the packet handler and the bonding mode changing (the mode can only change when the device is closed). Also updated the log message to include the reproduction and full commit ids. -J ]

Quoted 4 lines

Quoted 2 lines
Signed-off-by Greg Edwards <[email protected]>
Signed-off-by Jay Vosburgh <[email protected]>
Acked-by Andy Gospodarek <[email protected]>

This seems serious enough to put into net-2.6, so that's where I applied it.

Thanks!