All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] bonding: set device in RLB ARP packet handler
@ 2010-07-22 19:52 Greg Edwards
  2010-07-23 20:02 ` [PATCH net-next-2.6] " Jay Vosburgh
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Edwards @ 2010-07-22 19:52 UTC (permalink / raw)
  To: Jay Vosburgh; +Cc: bonding-devel, linux-kernel

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 <greg.edwards@hp.com>
---
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(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index df48307..8d7dfd2 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -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 */
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next-2.6] bonding: set device in RLB ARP packet handler
  2010-07-22 19:52 [PATCH] bonding: set device in RLB ARP packet handler Greg Edwards
@ 2010-07-23 20:02 ` Jay Vosburgh
  2010-07-23 20:26   ` Andy Gospodarek
  0 siblings, 1 reply; 9+ messages in thread
From: Jay Vosburgh @ 2010-07-23 20:02 UTC (permalink / raw)
  To: Greg Edwards; +Cc: netdev, David S. Miller, bonding-devel, linux-kernel


From: Greg Edwards <greg.edwards@hp.com>

After:

commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e
Author: Jay Vosburgh <fubar@us.ibm.com>
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 <andy@greyhouse.net>
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 ]
	
Signed-off-by: Greg Edwards <greg.edwards@hp.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

---

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

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index df48307..8d7dfd2 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -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 */
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next-2.6] bonding: set device in RLB ARP packet handler
  2010-07-23 20:02 ` [PATCH net-next-2.6] " Jay Vosburgh
@ 2010-07-23 20:26   ` Andy Gospodarek
  2010-07-25  3:38     ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Gospodarek @ 2010-07-23 20:26 UTC (permalink / raw)
  To: Jay Vosburgh
  Cc: Greg Edwards, netdev, David S. Miller, bonding-devel, linux-kernel

On Fri, Jul 23, 2010 at 01:02:04PM -0700, Jay Vosburgh wrote:
> 
> From: Greg Edwards <greg.edwards@hp.com>
> 
> After:
> 
> commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e
> Author: Jay Vosburgh <fubar@us.ibm.com>
> 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 <andy@greyhouse.net>
> 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 ]
> 	
> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>

> 
> ---
> 
>  drivers/net/bonding/bond_alb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index df48307..8d7dfd2 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -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 */
> -- 
> 1.7.0.4
> 
> --
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next-2.6] bonding: set device in RLB ARP packet handler
  2010-07-23 20:26   ` Andy Gospodarek
@ 2010-07-25  3:38     ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2010-07-25  3:38 UTC (permalink / raw)
  To: andy; +Cc: fubar, greg.edwards, netdev, bonding-devel, linux-kernel

From: Andy Gospodarek <andy@greyhouse.net>
Date: Fri, 23 Jul 2010 16:26:48 -0400

> On Fri, Jul 23, 2010 at 01:02:04PM -0700, Jay Vosburgh wrote:
>> 
>> From: Greg Edwards <greg.edwards@hp.com>
>> 
>> After:
>> 
>> commit 6146b1a4da98377e4abddc91ba5856bef8f23f1e
>> Author: Jay Vosburgh <fubar@us.ibm.com>
>> 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 <andy@greyhouse.net>
>> 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 ]
>> 	
>> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
>> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> 
> Acked-by: Andy Gospodarek <andy@greyhouse.net>

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

Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bonding: set device in RLB ARP packet handler
  2010-07-23 19:59   ` Greg Edwards
@ 2010-07-23 20:25     ` Andy Gospodarek
  -1 siblings, 0 replies; 9+ messages in thread
From: Andy Gospodarek @ 2010-07-23 20:25 UTC (permalink / raw)
  To: Greg Edwards
  Cc: Andy Gospodarek, Jay Vosburgh, bonding-devel, linux-kernel, netdev

On Fri, Jul 23, 2010 at 01:59:47PM -0600, Greg Edwards wrote:
> On Fri, Jul 23, 2010 at 07:34:56PM +0000, Andy Gospodarek wrote:
> > On Thu, Jul 22, 2010 at 3:52 PM, Greg Edwards <greg.edwards@hp.com> wrote:
> >> 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.
> >
> > I see this problem as well, but I would propose to fix it another way to
> > not alter the receive path so close to the release of 2.6.35 and to
> > catch this for 802.3ad bonds as well.
> 
> Is the problem demonstrable with 802.3ad bonds?  bond_register_lacpdu()
> sets pk_type->dev = bond->dev.
> 

Doh!  You are right.

Plus Jay just posted your patch again, so we can go with that solution.

> >> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
> >> ---
> >> 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.
> >>
> >
> > bonding: make sure mode-specific handlers handle appropriate frames
> >
> > This patch will exit out of rlb_arp_recv and bond_3ad_lacpdu_recv early
> > if the bond receiving the frame isn't using that mode.
> 
> I had originally thought of doing something like this, but it didn't
> seem as clean.  I don't have strong feelings one way or the other,
> though.
> 
> Greg
> --
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bonding: set device in RLB ARP packet handler
@ 2010-07-23 20:25     ` Andy Gospodarek
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Gospodarek @ 2010-07-23 20:25 UTC (permalink / raw)
  To: Greg Edwards
  Cc: Andy Gospodarek, Jay Vosburgh, bonding-devel, linux-kernel, netdev

On Fri, Jul 23, 2010 at 01:59:47PM -0600, Greg Edwards wrote:
> On Fri, Jul 23, 2010 at 07:34:56PM +0000, Andy Gospodarek wrote:
> > On Thu, Jul 22, 2010 at 3:52 PM, Greg Edwards <greg.edwards@hp.com> wrote:
> >> 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.
> >
> > I see this problem as well, but I would propose to fix it another way to
> > not alter the receive path so close to the release of 2.6.35 and to
> > catch this for 802.3ad bonds as well.
> 
> Is the problem demonstrable with 802.3ad bonds?  bond_register_lacpdu()
> sets pk_type->dev = bond->dev.
> 

Doh!  You are right.

Plus Jay just posted your patch again, so we can go with that solution.

> >> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
> >> ---
> >> 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.
> >>
> >
> > bonding: make sure mode-specific handlers handle appropriate frames
> >
> > This patch will exit out of rlb_arp_recv and bond_3ad_lacpdu_recv early
> > if the bond receiving the frame isn't using that mode.
> 
> I had originally thought of doing something like this, but it didn't
> seem as clean.  I don't have strong feelings one way or the other,
> though.
> 
> Greg
> --
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bonding: set device in RLB ARP packet handler
  2010-07-23 19:34 [PATCH] " Andy Gospodarek
@ 2010-07-23 19:59   ` Greg Edwards
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Edwards @ 2010-07-23 19:59 UTC (permalink / raw)
  To: Andy Gospodarek; +Cc: Jay Vosburgh, bonding-devel, linux-kernel, netdev

On Fri, Jul 23, 2010 at 07:34:56PM +0000, Andy Gospodarek wrote:
> On Thu, Jul 22, 2010 at 3:52 PM, Greg Edwards <greg.edwards@hp.com> wrote:
>> 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.
>
> I see this problem as well, but I would propose to fix it another way to
> not alter the receive path so close to the release of 2.6.35 and to
> catch this for 802.3ad bonds as well.

Is the problem demonstrable with 802.3ad bonds?  bond_register_lacpdu()
sets pk_type->dev = bond->dev.

>> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
>> ---
>> 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.
>>
>
> bonding: make sure mode-specific handlers handle appropriate frames
>
> This patch will exit out of rlb_arp_recv and bond_3ad_lacpdu_recv early
> if the bond receiving the frame isn't using that mode.

I had originally thought of doing something like this, but it didn't
seem as clean.  I don't have strong feelings one way or the other,
though.

Greg

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bonding: set device in RLB ARP packet handler
@ 2010-07-23 19:59   ` Greg Edwards
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Edwards @ 2010-07-23 19:59 UTC (permalink / raw)
  To: Andy Gospodarek; +Cc: Jay Vosburgh, bonding-devel, linux-kernel, netdev

On Fri, Jul 23, 2010 at 07:34:56PM +0000, Andy Gospodarek wrote:
> On Thu, Jul 22, 2010 at 3:52 PM, Greg Edwards <greg.edwards@hp.com> wrote:
>> 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.
>
> I see this problem as well, but I would propose to fix it another way to
> not alter the receive path so close to the release of 2.6.35 and to
> catch this for 802.3ad bonds as well.

Is the problem demonstrable with 802.3ad bonds?  bond_register_lacpdu()
sets pk_type->dev = bond->dev.

>> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
>> ---
>> 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.
>>
>
> bonding: make sure mode-specific handlers handle appropriate frames
>
> This patch will exit out of rlb_arp_recv and bond_3ad_lacpdu_recv early
> if the bond receiving the frame isn't using that mode.

I had originally thought of doing something like this, but it didn't
seem as clean.  I don't have strong feelings one way or the other,
though.

Greg

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bonding: set device in RLB ARP packet handler
@ 2010-07-23 19:34 Andy Gospodarek
  2010-07-23 19:59   ` Greg Edwards
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Gospodarek @ 2010-07-23 19:34 UTC (permalink / raw)
  To: Greg Edwards; +Cc: Jay Vosburgh, bonding-devel, linux-kernel, netdev

On Thu, Jul 22, 2010 at 3:52 PM, Greg Edwards <greg.edwards@hp.com> wrote:
> 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.
>

I see this problem as well, but I would propose to fix it another way to
not alter the receive path so close to the release of 2.6.35 and to
catch this for 802.3ad bonds as well.

> Signed-off-by: Greg Edwards <greg.edwards@hp.com>
> ---
> 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.
>

bonding: make sure mode-specific handlers handle appropriate frames

This patch will exit out of rlb_arp_recv and bond_3ad_lacpdu_recv early
if the bond receiving the frame isn't using that mode.

Fixes problem reported by Greg Edwards <greg.edwards@hp.com>.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

---
 drivers/net/bonding/bond_3ad.c |    3 ++-
 drivers/net/bonding/bond_alb.c |    6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 822f586..cf7b08d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2463,7 +2463,8 @@ int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct pac
 	struct slave *slave = NULL;
 	int ret = NET_RX_DROP;
 
-	if (!(dev->flags & IFF_MASTER))
+	if (!(dev->flags & IFF_MASTER) ||
+	    (bond->params.mode != BOND_MODE_8023AD))
 		goto out;
 
 	read_lock(&bond->lock);
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index df48307..a82e38c 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -353,7 +353,7 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
 
 static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
 {
-	struct bonding *bond;
+	struct bonding *bond = netdev_priv(bond_dev);
 	struct arp_pkt *arp = (struct arp_pkt *)skb->data;
 	int res = NET_RX_DROP;
 
@@ -361,7 +361,8 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct
 		bond_dev = vlan_dev_real_dev(bond_dev);
 
 	if (!(bond_dev->priv_flags & IFF_BONDING) ||
-	    !(bond_dev->flags & IFF_MASTER))
+	    !(bond_dev->flags & IFF_MASTER) ||
+	    (bond->params.mode != BOND_MODE_ALB))
 		goto out;
 
 	if (!arp) {
@@ -376,7 +377,6 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct
 
 	if (arp->op_code == htons(ARPOP_REPLY)) {
 		/* update rx hash table for this ARP */
-		bond = netdev_priv(bond_dev);
 		rlb_update_entry_from_arp(bond, arp);
 		pr_debug("Server received an ARP Reply from client\n");
 	}
-- 
1.6.2.5




^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-07-25  3:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-22 19:52 [PATCH] bonding: set device in RLB ARP packet handler Greg Edwards
2010-07-23 20:02 ` [PATCH net-next-2.6] " Jay Vosburgh
2010-07-23 20:26   ` Andy Gospodarek
2010-07-25  3:38     ` David Miller
2010-07-23 19:34 [PATCH] " Andy Gospodarek
2010-07-23 19:59 ` Greg Edwards
2010-07-23 19:59   ` Greg Edwards
2010-07-23 20:25   ` Andy Gospodarek
2010-07-23 20:25     ` Andy Gospodarek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.