All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
@ 2009-03-24  9:54 Stanichenko Marat
  0 siblings, 0 replies; 12+ messages in thread
From: Stanichenko Marat @ 2009-03-24  9:54 UTC (permalink / raw)
  To: linux-kernel

> Thu, Mar 19, 2009 at 11:21:43AM CET, davem@davemloft.net wrote:
>>From: Jiri Pirko <jpirko@redhat.com>
>>Date: Thu, 19 Mar 2009 09:44:45 +0100
>>
>>> Yes I was looking at this thing yesterday (uc_list). But this list serves
>>> to different purpose. Do you think that it will be correct to use it for this? I
>>> would maybe like to make a new list similar to this for our purpose
>>> (say addr_list). I think it would be more correct.
>>
>>Whatever you do with that list privately inside of the bonding
>>driver should be fine.
> Well I do not need it only inside the bonding driver. I want bridge to use this
> list when adding a device in it and get mac addresses from there into its
> hashlist (to recognize these addresses as local).
Please correct me if I understand you improperly. You're going to mark all mac 
addresses that belong to slaves as "local" when adding a bond device to the 
bridge, aren't you? The only thing I'd like to notice (this might be an obvious 
one): a packet that is pushed out from one slave might reach the host through 
another slave. Considering all slaves as "local" in bridge code might lead to 
numerous messages "received packet with own address as source address".

Please CC me personally when answering this message.

Thanks,
Marat.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-19  8:50           ` Patrick McHardy
@ 2009-03-19 16:31             ` Jiri Pirko
  0 siblings, 0 replies; 12+ messages in thread
From: Jiri Pirko @ 2009-03-19 16:31 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: David Miller, shemminger, linux-kernel, netdev, jgarzik, bridge,
	fubar, bonding-devel

Thu, Mar 19, 2009 at 09:50:03AM CET, kaber@trash.net wrote:
> David Miller wrote:
>> From: Jiri Pirko <jpirko@redhat.com>
>> Date: Mon, 16 Mar 2009 12:11:28 +0100
>>
>>> I can see two solutions. Either like my patch or somehow allow bridge to know
>>> more MAC addressses per port (maybe netdev can be changed to know more then
>>> one MAC address).
>>>
>>> Any thoughts?
>>
>> The netdev struct already supports having a list of multiple unicast
>> MAC addresses, it can probably be used and inspected for this.
>>
>> I'll hold off on your patch until we make some more progress on
>> this discussion.
>
> From reading the balance-alb description, I get the impression that this
> mode is simply not meant to be used with bridging:
>
> 		Adaptive load balancing: includes balance-tlb plus
> 		receive load balancing (rlb) for IPV4 traffic, and
> 		does not require any special switch support.  The
> 		receive load balancing is achieved by ARP negotiation.
> 		The bonding driver intercepts the ARP Replies sent by
> 		the local system on their way out and overwrites the
> 		source hardware address with the unique hardware
> 		address of one of the slaves in the bond such that
> 		different peers use different hardware addresses for
> 		the server.
>
> In any case I'd tend to say that if bond-alb mode mangles outgoing MAC
> addresses, it should restore the original one for received packets
> and keep the hacks local to bonding.

To let bonding driver to resolve this I think there will be needed some kind of
hook in netif_receive_skb() as for example bridge has. I would rather do this
more general and transparent.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-19 10:21             ` David Miller
@ 2009-03-19 11:19               ` Jiri Pirko
  0 siblings, 0 replies; 12+ messages in thread
From: Jiri Pirko @ 2009-03-19 11:19 UTC (permalink / raw)
  To: David Miller
  Cc: shemminger, linux-kernel, netdev, jgarzik, bridge, fubar, bonding-devel

Thu, Mar 19, 2009 at 11:21:43AM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jpirko@redhat.com>
>Date: Thu, 19 Mar 2009 09:44:45 +0100
>
>> Yes I was looking at this thing yesterday (uc_list). But this list serves
>> to different purpose. Do you think that it will be correct to use it for this? I
>> would maybe like to make a new list similar to this for our purpose
>> (say addr_list). I think it would be more correct.
>
>Whatever you do with that list privately inside of the bonding
>driver should be fine.
Well I do not need it only inside the bonding driver. I want bridge to use this
list when adding a device in it and get mac addresses from there into its
hashlist (to recognize these addresses as local).
>
>It might upset something in the generic code if you don't clean
>it up before deregistration of the bonding device, so just be
>tidy.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-19  8:44           ` Jiri Pirko
@ 2009-03-19 10:21             ` David Miller
  2009-03-19 11:19               ` Jiri Pirko
  0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2009-03-19 10:21 UTC (permalink / raw)
  To: jpirko
  Cc: shemminger, linux-kernel, netdev, jgarzik, bridge, fubar, bonding-devel

From: Jiri Pirko <jpirko@redhat.com>
Date: Thu, 19 Mar 2009 09:44:45 +0100

> Yes I was looking at this thing yesterday (uc_list). But this list serves
> to different purpose. Do you think that it will be correct to use it for this? I
> would maybe like to make a new list similar to this for our purpose
> (say addr_list). I think it would be more correct.

Whatever you do with that list privately inside of the bonding
driver should be fine.

It might upset something in the generic code if you don't clean
it up before deregistration of the bonding device, so just be
tidy.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-19  6:20         ` David Miller
  2009-03-19  8:44           ` Jiri Pirko
@ 2009-03-19  8:50           ` Patrick McHardy
  2009-03-19 16:31             ` Jiri Pirko
  1 sibling, 1 reply; 12+ messages in thread
From: Patrick McHardy @ 2009-03-19  8:50 UTC (permalink / raw)
  To: David Miller
  Cc: jpirko, shemminger, linux-kernel, netdev, jgarzik, bridge, fubar,
	bonding-devel

David Miller wrote:
> From: Jiri Pirko <jpirko@redhat.com>
> Date: Mon, 16 Mar 2009 12:11:28 +0100
> 
>> I can see two solutions. Either like my patch or somehow allow bridge to know
>> more MAC addressses per port (maybe netdev can be changed to know more then
>> one MAC address).
>>
>> Any thoughts?
> 
> The netdev struct already supports having a list of multiple unicast
> MAC addresses, it can probably be used and inspected for this.
> 
> I'll hold off on your patch until we make some more progress on
> this discussion.

 From reading the balance-alb description, I get the impression that this
mode is simply not meant to be used with bridging:

		Adaptive load balancing: includes balance-tlb plus
		receive load balancing (rlb) for IPV4 traffic, and
		does not require any special switch support.  The
		receive load balancing is achieved by ARP negotiation.
		The bonding driver intercepts the ARP Replies sent by
		the local system on their way out and overwrites the
		source hardware address with the unique hardware
		address of one of the slaves in the bond such that
		different peers use different hardware addresses for
		the server.

In any case I'd tend to say that if bond-alb mode mangles outgoing MAC
addresses, it should restore the original one for received packets
and keep the hacks local to bonding.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-19  6:20         ` David Miller
@ 2009-03-19  8:44           ` Jiri Pirko
  2009-03-19 10:21             ` David Miller
  2009-03-19  8:50           ` Patrick McHardy
  1 sibling, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2009-03-19  8:44 UTC (permalink / raw)
  To: David Miller
  Cc: shemminger, linux-kernel, netdev, jgarzik, bridge, fubar, bonding-devel

Thu, Mar 19, 2009 at 07:20:03AM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jpirko@redhat.com>
>Date: Mon, 16 Mar 2009 12:11:28 +0100
>
>> I can see two solutions. Either like my patch or somehow allow bridge to know
>> more MAC addressses per port (maybe netdev can be changed to know more then
>> one MAC address).
>> 
>> Any thoughts?
>
>The netdev struct already supports having a list of multiple unicast
>MAC addresses, it can probably be used and inspected for this.
Yes I was looking at this thing yesterday (uc_list). But this list serves
to different purpose. Do you think that it will be correct to use it for this? I
would maybe like to make a new list similar to this for our purpose
(say addr_list). I think it would be more correct.

Eventually in the furute we would use this list as a primary place to store
device address instead of dev_addr value and make it more general (as device
generally may have more adresses). Just a thought...

>
>I'll hold off on your patch until we make some more progress on
>this discussion.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-16 11:11       ` Jiri Pirko
@ 2009-03-19  6:20         ` David Miller
  2009-03-19  8:44           ` Jiri Pirko
  2009-03-19  8:50           ` Patrick McHardy
  0 siblings, 2 replies; 12+ messages in thread
From: David Miller @ 2009-03-19  6:20 UTC (permalink / raw)
  To: jpirko
  Cc: shemminger, linux-kernel, netdev, jgarzik, bridge, fubar, bonding-devel

From: Jiri Pirko <jpirko@redhat.com>
Date: Mon, 16 Mar 2009 12:11:28 +0100

> I can see two solutions. Either like my patch or somehow allow bridge to know
> more MAC addressses per port (maybe netdev can be changed to know more then
> one MAC address).
> 
> Any thoughts?

The netdev struct already supports having a list of multiple unicast
MAC addresses, it can probably be used and inspected for this.

I'll hold off on your patch until we make some more progress on
this discussion.

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-15 23:12     ` Stephen Hemminger
@ 2009-03-16 11:11       ` Jiri Pirko
  2009-03-19  6:20         ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2009-03-16 11:11 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: linux-kernel, netdev, jgarzik, davem, bridge, fubar, bonding-devel

Mon, Mar 16, 2009 at 12:12:17AM CET, shemminger@linux-foundation.org wrote:
>On Sat, 14 Mar 2009 10:49:11 +0100
>Jiri Pirko <jpirko@redhat.com> wrote:
>
>> Sat, Mar 14, 2009 at 06:39:32AM CET, shemminger@linux-foundation.org wrote:
>> >On Fri, 13 Mar 2009 19:33:04 +0100
>> >Jiri Pirko <jpirko@redhat.com> wrote:
>> >
>> >> Hi all.
>> >> 
>> >> This is only a draft of patch to consult. I'm aware that it should be divided
>> >> into multiple patches. I want to know opinion from you folks.
>> >> 
>> >> The problem is described in following bugzilla:
>> >> https://bugzilla.redhat.com/show_bug.cgi?id=487763
>> >> 
>> >> Basically here's what's going on. In every mode, bonding interface uses the same
>> >> mac address for all enslaved devices. Except for mode balance-alb. When you put
>> >> this kind of bond device into a bridge it will only add one of mac adresses into
>> >> a hash list of mac addresses, say X. This mac address is marked as local. But
>> >> this bonding interface also has mac address Y. Now then packet arrives with
>> >> destination address Y, this address is not marked as local and the packed looks
>> >> like it needs to be forwarded. This packet is then lost which is wrong.
>> >> 
>> >> Notice that interfaces can be added and removed from bond while it is in bridge.
>> >> Therefore I introduce another function pointer in struct net_device_ops -
>> >> ndo_check_mac_address. This function when it's implemented should check passed
>> >> mac address against the one set in device. I'm using this in bonding driver when
>> >> the bond is in mode balance-alb to walk thru all slaves and checking if any of
>> >> them equals passed address.
>> >> 
>> >> Then in bridge function br_handle_frame_finish() I'm using ndo_check_mac_address
>> >> to recognize the destination mac address as local.
>> >> 
>> >> Please look at this and tell me what you think about it.
>> >> 
>> >> Thanks
>> >> 
>> >> Jirka
>> >>
>> >
>> >A better and more general way to do this have the dev_set_mac_address
>> >function check the return of the notifier and unwind. Then any protocol
>> >can easily prevent address from changing.
>> 
>> Can you please describe this thougth a bit more? I can't understand it now...
>> 
>> Thanks
>> 
>> Jirka
>
>Something like this:
>
>--- a/net/core/dev.c	2009-03-15 15:55:02.098126056 -0700
>+++ b/net/core/dev.c	2009-03-15 16:02:43.999251305 -0700
>@@ -3830,6 +3830,7 @@ int dev_set_mac_address(struct net_devic
> {
> 	const struct net_device_ops *ops = dev->netdev_ops;
> 	int err;
>+	char save_addr[MAX_ADDR_LEN];
> 
> 	if (!ops->ndo_set_mac_address)
> 		return -EOPNOTSUPP;
>@@ -3837,9 +3838,17 @@ int dev_set_mac_address(struct net_devic
> 		return -EINVAL;
> 	if (!netif_device_present(dev))
> 		return -ENODEV;
>+
>+	memcpy(save_addr, dev->dev_addr, dev->addr_len);
> 	err = ops->ndo_set_mac_address(dev, sa);
>-	if (!err)
>-		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
>+	if (err)
>+		return err;
>+
>+	err = call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
>+	if (err) {
>+		memcpy(sa->sa_data, save_addr, dev->addr_len);
>+		ops->ndo_set_mac_address(dev, sa);
>+	}
> 	return err;
> }
> 
>
>And something like this:
>
>--- a/drivers/net/bonding/bond_main.c	2009-03-15 16:03:53.909000973 -0700
>+++ b/drivers/net/bonding/bond_main.c	2009-03-15 16:11:43.227127031 -0700
>@@ -3534,6 +3534,7 @@ static int bond_slave_netdev_event(unsig
> {
> 	struct net_device *bond_dev = slave_dev->master;
> 	struct bonding *bond = netdev_priv(bond_dev);
>+	int err;
> 
> 	switch (event) {
> 	case NETDEV_UNREGISTER:
>@@ -3570,6 +3571,15 @@ static int bond_slave_netdev_event(unsig
> 		 * servitude.
> 		 */
> 		break;
>+	case NETDEV_CHANGEADDR:
>+		if (bond->params.mode == BOND_MODE_ALB)
>+			err = bond_alb_check_mac_address(bond);
>+		else if (compare_ether_addr(bond_dev->dev_addr, addr) != 0)
>+			err = -EINVAL;
>+
>+		if (err)
>+			return notifier_from_errno(err);
>+		break;
> 	case NETDEV_CHANGENAME:
> 		/*
> 		 * TODO: handle changing the primary's name
>
Yes, I think the changing mac address of slaves should be also handled by
bonding driver. But my patch fixes a different issue. See, unlike in any other
bonding modes, in balance-alb mode incoming packets have multiple MAC adresses
(of any of enslaved devices). This causes problem because bridge only recognize
one of them (the mac of master which is the mac on one of the slaves) as local -
the other MAC's are not recognized as they are a part of port and therefore
handled as general MAC adresses. This is the problem.

I can see two solutions. Either like my patch or somehow allow bridge to know
more MAC addressses per port (maybe netdev can be changed to know more then
one MAC address).

Any thoughts?

Thanks

Jirka
>
>

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-14  9:49   ` Jiri Pirko
@ 2009-03-15 23:12     ` Stephen Hemminger
  2009-03-16 11:11       ` Jiri Pirko
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2009-03-15 23:12 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: linux-kernel, netdev, jgarzik, davem, bridge, fubar, bonding-devel

On Sat, 14 Mar 2009 10:49:11 +0100
Jiri Pirko <jpirko@redhat.com> wrote:

> Sat, Mar 14, 2009 at 06:39:32AM CET, shemminger@linux-foundation.org wrote:
> >On Fri, 13 Mar 2009 19:33:04 +0100
> >Jiri Pirko <jpirko@redhat.com> wrote:
> >
> >> Hi all.
> >> 
> >> This is only a draft of patch to consult. I'm aware that it should be divided
> >> into multiple patches. I want to know opinion from you folks.
> >> 
> >> The problem is described in following bugzilla:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=487763
> >> 
> >> Basically here's what's going on. In every mode, bonding interface uses the same
> >> mac address for all enslaved devices. Except for mode balance-alb. When you put
> >> this kind of bond device into a bridge it will only add one of mac adresses into
> >> a hash list of mac addresses, say X. This mac address is marked as local. But
> >> this bonding interface also has mac address Y. Now then packet arrives with
> >> destination address Y, this address is not marked as local and the packed looks
> >> like it needs to be forwarded. This packet is then lost which is wrong.
> >> 
> >> Notice that interfaces can be added and removed from bond while it is in bridge.
> >> Therefore I introduce another function pointer in struct net_device_ops -
> >> ndo_check_mac_address. This function when it's implemented should check passed
> >> mac address against the one set in device. I'm using this in bonding driver when
> >> the bond is in mode balance-alb to walk thru all slaves and checking if any of
> >> them equals passed address.
> >> 
> >> Then in bridge function br_handle_frame_finish() I'm using ndo_check_mac_address
> >> to recognize the destination mac address as local.
> >> 
> >> Please look at this and tell me what you think about it.
> >> 
> >> Thanks
> >> 
> >> Jirka
> >>
> >
> >A better and more general way to do this have the dev_set_mac_address
> >function check the return of the notifier and unwind. Then any protocol
> >can easily prevent address from changing.
> 
> Can you please describe this thougth a bit more? I can't understand it now...
> 
> Thanks
> 
> Jirka

Something like this:

--- a/net/core/dev.c	2009-03-15 15:55:02.098126056 -0700
+++ b/net/core/dev.c	2009-03-15 16:02:43.999251305 -0700
@@ -3830,6 +3830,7 @@ int dev_set_mac_address(struct net_devic
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int err;
+	char save_addr[MAX_ADDR_LEN];
 
 	if (!ops->ndo_set_mac_address)
 		return -EOPNOTSUPP;
@@ -3837,9 +3838,17 @@ int dev_set_mac_address(struct net_devic
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+
+	memcpy(save_addr, dev->dev_addr, dev->addr_len);
 	err = ops->ndo_set_mac_address(dev, sa);
-	if (!err)
-		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+	if (err)
+		return err;
+
+	err = call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+	if (err) {
+		memcpy(sa->sa_data, save_addr, dev->addr_len);
+		ops->ndo_set_mac_address(dev, sa);
+	}
 	return err;
 }
 

And something like this:

--- a/drivers/net/bonding/bond_main.c	2009-03-15 16:03:53.909000973 -0700
+++ b/drivers/net/bonding/bond_main.c	2009-03-15 16:11:43.227127031 -0700
@@ -3534,6 +3534,7 @@ static int bond_slave_netdev_event(unsig
 {
 	struct net_device *bond_dev = slave_dev->master;
 	struct bonding *bond = netdev_priv(bond_dev);
+	int err;
 
 	switch (event) {
 	case NETDEV_UNREGISTER:
@@ -3570,6 +3571,15 @@ static int bond_slave_netdev_event(unsig
 		 * servitude.
 		 */
 		break;
+	case NETDEV_CHANGEADDR:
+		if (bond->params.mode == BOND_MODE_ALB)
+			err = bond_alb_check_mac_address(bond);
+		else if (compare_ether_addr(bond_dev->dev_addr, addr) != 0)
+			err = -EINVAL;
+
+		if (err)
+			return notifier_from_errno(err);
+		break;
 	case NETDEV_CHANGENAME:
 		/*
 		 * TODO: handle changing the primary's name




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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-14  5:39 ` Stephen Hemminger
@ 2009-03-14  9:49   ` Jiri Pirko
  2009-03-15 23:12     ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2009-03-14  9:49 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: linux-kernel, netdev, jgarzik, davem, bridge, fubar, bonding-devel

Sat, Mar 14, 2009 at 06:39:32AM CET, shemminger@linux-foundation.org wrote:
>On Fri, 13 Mar 2009 19:33:04 +0100
>Jiri Pirko <jpirko@redhat.com> wrote:
>
>> Hi all.
>> 
>> This is only a draft of patch to consult. I'm aware that it should be divided
>> into multiple patches. I want to know opinion from you folks.
>> 
>> The problem is described in following bugzilla:
>> https://bugzilla.redhat.com/show_bug.cgi?id=487763
>> 
>> Basically here's what's going on. In every mode, bonding interface uses the same
>> mac address for all enslaved devices. Except for mode balance-alb. When you put
>> this kind of bond device into a bridge it will only add one of mac adresses into
>> a hash list of mac addresses, say X. This mac address is marked as local. But
>> this bonding interface also has mac address Y. Now then packet arrives with
>> destination address Y, this address is not marked as local and the packed looks
>> like it needs to be forwarded. This packet is then lost which is wrong.
>> 
>> Notice that interfaces can be added and removed from bond while it is in bridge.
>> Therefore I introduce another function pointer in struct net_device_ops -
>> ndo_check_mac_address. This function when it's implemented should check passed
>> mac address against the one set in device. I'm using this in bonding driver when
>> the bond is in mode balance-alb to walk thru all slaves and checking if any of
>> them equals passed address.
>> 
>> Then in bridge function br_handle_frame_finish() I'm using ndo_check_mac_address
>> to recognize the destination mac address as local.
>> 
>> Please look at this and tell me what you think about it.
>> 
>> Thanks
>> 
>> Jirka
>>
>
>A better and more general way to do this have the dev_set_mac_address
>function check the return of the notifier and unwind. Then any protocol
>can easily prevent address from changing.

Can you please describe this thougth a bit more? I can't understand it now...

Thanks

Jirka

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

* Re: [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
  2009-03-13 18:33 Jiri Pirko
@ 2009-03-14  5:39 ` Stephen Hemminger
  2009-03-14  9:49   ` Jiri Pirko
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2009-03-14  5:39 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: linux-kernel, netdev, jgarzik, davem, bridge, fubar,
	bonding-devel, netdev

On Fri, 13 Mar 2009 19:33:04 +0100
Jiri Pirko <jpirko@redhat.com> wrote:

> Hi all.
> 
> This is only a draft of patch to consult. I'm aware that it should be divided
> into multiple patches. I want to know opinion from you folks.
> 
> The problem is described in following bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=487763
> 
> Basically here's what's going on. In every mode, bonding interface uses the same
> mac address for all enslaved devices. Except for mode balance-alb. When you put
> this kind of bond device into a bridge it will only add one of mac adresses into
> a hash list of mac addresses, say X. This mac address is marked as local. But
> this bonding interface also has mac address Y. Now then packet arrives with
> destination address Y, this address is not marked as local and the packed looks
> like it needs to be forwarded. This packet is then lost which is wrong.
> 
> Notice that interfaces can be added and removed from bond while it is in bridge.
> Therefore I introduce another function pointer in struct net_device_ops -
> ndo_check_mac_address. This function when it's implemented should check passed
> mac address against the one set in device. I'm using this in bonding driver when
> the bond is in mode balance-alb to walk thru all slaves and checking if any of
> them equals passed address.
> 
> Then in bridge function br_handle_frame_finish() I'm using ndo_check_mac_address
> to recognize the destination mac address as local.
> 
> Please look at this and tell me what you think about it.
> 
> Thanks
> 
> Jirka
>

A better and more general way to do this have the dev_set_mac_address
function check the return of the notifier and unwind. Then any protocol
can easily prevent address from changing.

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

* [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge
@ 2009-03-13 18:33 Jiri Pirko
  2009-03-14  5:39 ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2009-03-13 18:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: netdev, jgarzik, davem, shemminger, bridge, fubar, bonding-devel

Hi all.

This is only a draft of patch to consult. I'm aware that it should be divided
into multiple patches. I want to know opinion from you folks.

The problem is described in following bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=487763

Basically here's what's going on. In every mode, bonding interface uses the same
mac address for all enslaved devices. Except for mode balance-alb. When you put
this kind of bond device into a bridge it will only add one of mac adresses into
a hash list of mac addresses, say X. This mac address is marked as local. But
this bonding interface also has mac address Y. Now then packet arrives with
destination address Y, this address is not marked as local and the packed looks
like it needs to be forwarded. This packet is then lost which is wrong.

Notice that interfaces can be added and removed from bond while it is in bridge.
Therefore I introduce another function pointer in struct net_device_ops -
ndo_check_mac_address. This function when it's implemented should check passed
mac address against the one set in device. I'm using this in bonding driver when
the bond is in mode balance-alb to walk thru all slaves and checking if any of
them equals passed address.

Then in bridge function br_handle_frame_finish() I'm using ndo_check_mac_address
to recognize the destination mac address as local.

Please look at this and tell me what you think about it.

Thanks

Jirka


Signed-off-by: Jiri Pirko <jpirko@redhat.com>

 drivers/net/bonding/bond_alb.c  |   17 +++++++++++++++++
 drivers/net/bonding/bond_alb.h  |    1 +
 drivers/net/bonding/bond_main.c |   11 +++++++++++
 include/linux/netdevice.h       |    7 +++++++
 net/bridge/br_input.c           |    5 ++++-
 5 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 27fb7f5..b7bcee0 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1762,6 +1762,23 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 	return 0;
 }
 
+int bond_alb_check_mac_address(struct net_device *bond_dev, void *addr)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+	struct slave *slave = NULL;
+	int ret = !0;
+	int i;
+
+	read_lock(&bond->lock);
+	bond_for_each_slave(bond, slave, i) {
+		ret = compare_ether_addr(slave->perm_hwaddr, addr);
+		if (!ret)
+			break;
+	}
+	read_unlock(&bond->lock);
+	return ret;
+}
+
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
 	if (bond->alb_info.current_alb_vlan &&
diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
index 50968f8..5e39bda 100644
--- a/drivers/net/bonding/bond_alb.h
+++ b/drivers/net/bonding/bond_alb.h
@@ -127,6 +127,7 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
 int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev);
 void bond_alb_monitor(struct work_struct *);
 int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
+int bond_alb_check_mac_address(struct net_device *bond_dev, void *addr);
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
 #endif /* __BOND_ALB_H__ */
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e0578fe..fbff338 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4279,6 +4279,16 @@ unwind:
 	return res;
 }
 
+static int bond_check_mac_address(struct net_device *bond_dev, void *addr)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+
+	if (bond->params.mode == BOND_MODE_ALB)
+		return bond_alb_check_mac_address(bond_dev, addr);
+
+	return compare_ether_addr(bond_dev->dev_addr, addr);
+}
+
 static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
@@ -4576,6 +4586,7 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_set_multicast_list	= bond_set_multicast_list,
 	.ndo_change_mtu		= bond_change_mtu,
 	.ndo_set_mac_address 	= bond_set_mac_address,
+	.ndo_check_mac_address 	= bond_check_mac_address,
 	.ndo_neigh_setup	= bond_neigh_setup,
 	.ndo_vlan_rx_register	= bond_vlan_rx_register,
 	.ndo_vlan_rx_add_vid 	= bond_vlan_rx_add_vid,
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6593667..e75f691 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -491,6 +491,10 @@ struct netdev_queue {
  *	needs to be changed. If not this interface is not defined, the
  *	mac address can not be changed.
  *
+ * int (*ndo_check_mac_address)(struct net_device *dev, void *addr);
+ *	This function is called when the given Media Access Control address
+ *	needs to compared to the one set to the device.
+ *
  * int (*ndo_validate_addr)(struct net_device *dev);
  *	Test if Media Access Control address is valid for the device.
  *
@@ -554,6 +558,9 @@ struct net_device_ops {
 #define HAVE_SET_MAC_ADDR
 	int			(*ndo_set_mac_address)(struct net_device *dev,
 						       void *addr);
+#define HAVE_CHECK_MAC_ADDR
+	int			(*ndo_check_mac_address)(struct net_device *dev,
+						       void *addr);
 #define HAVE_VALIDATE_ADDR
 	int			(*ndo_validate_addr)(struct net_device *dev);
 #define HAVE_PRIVATE_IOCTL
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 30b8877..b071169 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -39,6 +39,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
 {
 	const unsigned char *dest = eth_hdr(skb)->h_dest;
 	struct net_bridge_port *p = rcu_dereference(skb->dev->br_port);
+	struct net_device *dev = p->dev;
 	struct net_bridge *br;
 	struct net_bridge_fdb_entry *dst;
 	struct sk_buff *skb2;
@@ -64,7 +65,9 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	if (is_multicast_ether_addr(dest)) {
 		br->dev->stats.multicast++;
 		skb2 = skb;
-	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
+	} else if (((dst = __br_fdb_get(br, dest)) && dst->is_local) ||
+		   (dev->netdev_ops->ndo_check_mac_address &&
+		    !dev->netdev_ops->ndo_check_mac_address(dev, (unsigned char *) dest))) {
 		skb2 = skb;
 		/* Do not forward the packet since it's local. */
 		skb = NULL;

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

end of thread, other threads:[~2009-03-24  9:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-24  9:54 [PATCH] bonding: allow bond in mode balance-alb to work properly in bridge Stanichenko Marat
  -- strict thread matches above, loose matches on Subject: below --
2009-03-13 18:33 Jiri Pirko
2009-03-14  5:39 ` Stephen Hemminger
2009-03-14  9:49   ` Jiri Pirko
2009-03-15 23:12     ` Stephen Hemminger
2009-03-16 11:11       ` Jiri Pirko
2009-03-19  6:20         ` David Miller
2009-03-19  8:44           ` Jiri Pirko
2009-03-19 10:21             ` David Miller
2009-03-19 11:19               ` Jiri Pirko
2009-03-19  8:50           ` Patrick McHardy
2009-03-19 16:31             ` Jiri Pirko

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.