The bonding driver should notify userspace of MAC address change

The bonding driver should notify userspace of MAC address change

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hello,

I'd like to file a feature request for the bonding driver. Currently,
there is no way for userspace to know whether the driver actually gets
a MAC address. This results in the fact that dhcpcd sends MAC-less DHCP
packets through bonding device if it is started before bond gets any
slaves.

I've reported that problem upstream [1], and the author suggested that
the bonding driver would have to notify the userspace about MAC address
change, suggesting using RTM_NEWLINK message.

I wanted to write a patch for that but I don't seem to see any
appropriate IFF_* flag for that particular kind of event. dhcpcd author
suggested using 'ifi->ifi_change = ~0U' but I'm not sure if it's
appropriate.

Could you either add such a kind of notification or give me a tip on
how to proceed with adding it? Thanks in advance.

[1] http://roy.marples.name/projects/dhcpcd/ticket/212

-- 
Best regards,
Michał Górny

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: The bonding driver should notify userspace of MAC address change

[Nicolas de Pesloüan]

Le 15/04/2011 18:44, Michał Górny a écrit :
> Hello,
>
> I'd like to file a feature request for the bonding driver. Currently,
> there is no way for userspace to know whether the driver actually gets
> a MAC address. This results in the fact that dhcpcd sends MAC-less DHCP
> packets through bonding device if it is started before bond gets any
> slaves.

A similar subject, involving bridge instead of bonding, was discussed a few weeks ago in this 
thread: http://marc.info/?l=linux-netdev&m=129939017116310&w=2

In particular, I suggested to apply Stephen's suggestion not only to bridge but also to bonding.

(http://marc.info/?l=linux-netdev&m=129948385024680&w=2)

A bonding device should not report link up to userspace until at least one slave is present and up.

And possibly, a bonding device should report link down if all slaves are down or all slave were removed.

Jay, Andy, does this sounds sensible to you?

> I've reported that problem upstream [1], and the author suggested that
> the bonding driver would have to notify the userspace about MAC address
> change, suggesting using RTM_NEWLINK message.
>
> I wanted to write a patch for that but I don't seem to see any
> appropriate IFF_* flag for that particular kind of event. dhcpcd author
> suggested using 'ifi->ifi_change = ~0U' but I'm not sure if it's
> appropriate.
>
> Could you either add such a kind of notification or give me a tip on
> how to proceed with adding it? Thanks in advance.
>
> [1] http://roy.marples.name/projects/dhcpcd/ticket/212

Re: The bonding driver should notify userspace of MAC address change

[Jay Vosburgh]

Nicolas de Pesloüan 	<nicolas.2p.debian@gmail.com> wrote:

>Le 15/04/2011 18:44, Michał Górny a écrit :
>> Hello,
>>
>> I'd like to file a feature request for the bonding driver. Currently,
>> there is no way for userspace to know whether the driver actually gets
>> a MAC address. This results in the fact that dhcpcd sends MAC-less DHCP
>> packets through bonding device if it is started before bond gets any
>> slaves.
>
>A similar subject, involving bridge instead of bonding, was discussed a
>few weeks ago in this thread:
>http://marc.info/?l=linux-netdev&m=129939017116310&w=2
>
>In particular, I suggested to apply Stephen's suggestion not only to bridge but also to bonding.
>
>(http://marc.info/?l=linux-netdev&m=129948385024680&w=2)
>
>A bonding device should not report link up to userspace until at least one slave is present and up.
>
>And possibly, a bonding device should report link down if all slaves are down or all slave were removed.
>
>Jay, Andy, does this sounds sensible to you?

	I was just reading their bug and doing an experiment; I don't
see that bonding reports carrier up until there's at least one slave
(even if it's configured up), e.g.,

# modprobe bonding
# ifconfig bond0 up
# cat /sys/class/net/bond0/carrier
0
# echo +eth5 > /sys/class/net/bond0/bonding/slaves
# cat /sys/class/net/bond0/carrier
1

	If there's a slave, there's a MAC assigned, since bond_enslave
sets the master's MAC before it calls bond_set_carrier.

	In bond_create, as soon as register_netdevice returns, we call
netif_carrier_off, and it stays off until bond_enslave runs
successfully.

	Is there some race window there between the register and the
netif_carrier_off?

	-J

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

[RFC net-next] bonding: notify when bonding device address changes

[Stephen Hemminger]

When a device changes its hardware address, it needs to call the network
device notifiers to inform protocols.

Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/bonding/bond_main.c	2011-04-15 11:21:02.142866195 -0700
+++ b/drivers/net/bonding/bond_main.c	2011-04-15 11:28:06.491408825 -0700
@@ -967,9 +967,11 @@ static void bond_do_fail_over_mac(struct
 
 	switch (bond->params.fail_over_mac) {
 	case BOND_FOM_ACTIVE:
-		if (new_active)
+		if (new_active) {
 			memcpy(bond->dev->dev_addr,  new_active->dev->dev_addr,
 			       new_active->dev->addr_len);
+			call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
+		}
 		break;
 	case BOND_FOM_FOLLOW:
 		/*
@@ -1386,6 +1388,7 @@ static int bond_sethwaddr(struct net_dev
 	pr_debug("slave_dev=%p\n", slave_dev);
 	pr_debug("slave_dev->addr_len=%d\n", slave_dev->addr_len);
 	memcpy(bond_dev->dev_addr, slave_dev->dev_addr, slave_dev->addr_len);
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
 	return 0;
 }
 
@@ -1644,10 +1647,11 @@ int bond_enslave(struct net_device *bond
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
-	if (is_zero_ether_addr(bond->dev->dev_addr))
+	if (is_zero_ether_addr(bond->dev->dev_addr)) {
 		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
 		       slave_dev->addr_len);
-
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
+	}
 
 	new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);
 	if (!new_slave) {
@@ -2067,6 +2071,7 @@ int bond_release(struct net_device *bond
 		 * to the mac address of the first slave
 		 */
 		memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
 
 		if (!bond->vlgrp) {
 			bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
@@ -2252,6 +2257,7 @@ static int bond_release_all(struct net_d
 	 * first slave
 	 */
 	memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
 
 	if (!bond->vlgrp) {
 		bond_dev->features |= NETIF_F_VLAN_CHALLENGED;

Re: The bonding driver should notify userspace of MAC address change

[Phil Oester]

On Fri, Apr 15, 2011 at 11:53:10AM -0700, Jay Vosburgh wrote:
> >A bonding device should not report link up to userspace until at least one slave is present and up.
> >
> >And possibly, a bonding device should report link down if all slaves are down or all slave were removed.
> >
> >Jay, Andy, does this sounds sensible to you?
> 
> 	I was just reading their bug and doing an experiment; I don't
> see that bonding reports carrier up until there's at least one slave
> (even if it's configured up), e.g.,
> 

This was only recently fixed - see e826eafa65c6f1f7c8db5a237556cebac57ebcc5
(bonding: Call netif_carrier_off after register_netdevice). Perhaps the
reporter is not using a recent kernel?

Phil 

Re: The bonding driver should notify userspace of MAC address change

[Nicolas de Pesloüan]

Le 15/04/2011 20:53, Jay Vosburgh a écrit :
> Nicolas de Pesloüan 	<nicolas.2p.debian@gmail.com>  wrote:
>
>> Le 15/04/2011 18:44, Michał Górny a écrit :
>>> Hello,
>>>
>>> I'd like to file a feature request for the bonding driver. Currently,
>>> there is no way for userspace to know whether the driver actually gets
>>> a MAC address. This results in the fact that dhcpcd sends MAC-less DHCP
>>> packets through bonding device if it is started before bond gets any
>>> slaves.
>>
>> A similar subject, involving bridge instead of bonding, was discussed a
>> few weeks ago in this thread:
>> http://marc.info/?l=linux-netdev&m=129939017116310&w=2
>>
>> In particular, I suggested to apply Stephen's suggestion not only to bridge but also to bonding.
>>
>> (http://marc.info/?l=linux-netdev&m=129948385024680&w=2)
>>
>> A bonding device should not report link up to userspace until at least one slave is present and up.
>>
>> And possibly, a bonding device should report link down if all slaves are down or all slave were removed.
>>
>> Jay, Andy, does this sounds sensible to you?
>
> 	I was just reading their bug and doing an experiment; I don't
> see that bonding reports carrier up until there's at least one slave
> (even if it's configured up), e.g.,
>
> # modprobe bonding
> # ifconfig bond0 up
> # cat /sys/class/net/bond0/carrier
> 0
> # echo +eth5>  /sys/class/net/bond0/bonding/slaves
> # cat /sys/class/net/bond0/carrier
> 1
>
> 	If there's a slave, there's a MAC assigned, since bond_enslave
> sets the master's MAC before it calls bond_set_carrier.
>
> 	In bond_create, as soon as register_netdevice returns, we call
> netif_carrier_off, and it stays off until bond_enslave runs
> successfully.

Agreed.

> 	Is there some race window there between the register and the
> netif_carrier_off?

It might be that dhcpd does not wait for link to be up before starting to send DHCP requests.

	Nicolas.

Re: The bonding driver should notify userspace of MAC address change

[Jay Vosburgh]

Phil Oester <kernel@linuxace.com> wrote:

>On Fri, Apr 15, 2011 at 11:53:10AM -0700, Jay Vosburgh wrote:
>> >A bonding device should not report link up to userspace until at least one slave is present and up.
>> >
>> >And possibly, a bonding device should report link down if all slaves are down or all slave were removed.
>> >
>> >Jay, Andy, does this sounds sensible to you?
>> 
>> 	I was just reading their bug and doing an experiment; I don't
>> see that bonding reports carrier up until there's at least one slave
>> (even if it's configured up), e.g.,
>> 
>
>This was only recently fixed - see e826eafa65c6f1f7c8db5a237556cebac57ebcc5
>(bonding: Call netif_carrier_off after register_netdevice). Perhaps the
>reporter is not using a recent kernel?

	Yah, I looked that up after I'd sent my prior email.  Since that
change went in only a month ago, maybe they don't have it.

	On the other hand, I did my test on a FC 14 kernel,
2.6.35.6-45.fc14, which claims to have been built in October 2010, and
it seemed to behave properly.

	-J

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

Re: [RFC net-next] bonding: notify when bonding device address changes

[Jay Vosburgh]

Stephen Hemminger <shemminger@vyatta.com> wrote:

>When a device changes its hardware address, it needs to call the network
>device notifiers to inform protocols.
>
>Compile tested only.

	We'll need to test this, I think.  If I'm not mistaken, I
believe that inetdev_event will issue gratuitous ARPs when it gets the
NETDEV_CHANGEADDR, and we need to make sure those are correct for all
cases.

>Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>--- a/drivers/net/bonding/bond_main.c	2011-04-15 11:21:02.142866195 -0700
>+++ b/drivers/net/bonding/bond_main.c	2011-04-15 11:28:06.491408825 -0700
>@@ -967,9 +967,11 @@ static void bond_do_fail_over_mac(struct
>
> 	switch (bond->params.fail_over_mac) {
> 	case BOND_FOM_ACTIVE:
>-		if (new_active)
>+		if (new_active) {
> 			memcpy(bond->dev->dev_addr,  new_active->dev->dev_addr,
> 			       new_active->dev->addr_len);
>+			call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
>+		}
> 		break;
> 	case BOND_FOM_FOLLOW:
> 		/*
>@@ -1386,6 +1388,7 @@ static int bond_sethwaddr(struct net_dev
> 	pr_debug("slave_dev=%p\n", slave_dev);
> 	pr_debug("slave_dev->addr_len=%d\n", slave_dev->addr_len);
> 	memcpy(bond_dev->dev_addr, slave_dev->dev_addr, slave_dev->addr_len);
>+	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
> 	return 0;
> }
>
>@@ -1644,10 +1647,11 @@ int bond_enslave(struct net_device *bond
>
> 	/* If this is the first slave, then we need to set the master's hardware
> 	 * address to be the same as the slave's. */
>-	if (is_zero_ether_addr(bond->dev->dev_addr))
>+	if (is_zero_ether_addr(bond->dev->dev_addr)) {
> 		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
> 		       slave_dev->addr_len);
>-
>+		call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
>+	}
>
> 	new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);
> 	if (!new_slave) {
>@@ -2067,6 +2071,7 @@ int bond_release(struct net_device *bond
> 		 * to the mac address of the first slave
> 		 */
> 		memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
>+		call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);

	This one in particular I'm not sure about; should the system
send a gratuitous ARP for a MAC address of all zeroes?

> 		if (!bond->vlgrp) {
> 			bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
>@@ -2252,6 +2257,7 @@ static int bond_release_all(struct net_d
> 	 * first slave
> 	 */
> 	memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
>+	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);

	Same comment for this one.

> 	if (!bond->vlgrp) {
> 		bond_dev->features |= NETIF_F_VLAN_CHALLENGED;

	-J

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

Re: The bonding driver should notify userspace of MAC address change

[Jay Vosburgh]

Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:

>Agreed.
>
>> 	Is there some race window there between the register and the
>> netif_carrier_off?
>
>It might be that dhcpd does not wait for link to be up before starting to send DHCP requests.

	It looks like it's not related to carrier state at all:

#212: dhcpcd requires restart to get an IP address for bonded interface
-----------------------+-----------------
  Reporter:  mgorny@…  |      Owner:  roy
      Type:  defect    |     Status:  new
  Priority:  major     |  Milestone:
 Component:  dhcpcd    |    Version:  5.1
Resolution:            |   Keywords:
-----------------------+-----------------

Comment (by roy):

 Sorry, the above isn't too clear.

 dhcpcd will read the hardware address when the interface is marked IFF_UP
 or when given RTM_NEWLINK with ifi->ifi_change = ~0U, the latter being
 sent by some drivers to tell userland that an interface characteristic has
 changed - like say a hardware address - if the driver supports such a
 change whilst still up. Normal behaviour is to mark device as DOWN before
 changing hardware address. bonding does this whilst marked UP, hence this
 issue.

 carrier going up / down is just that, it's not a signal to re-read the
 interface characteristics.


	Now this confuses me again; I thought that running the dhcp
client (dhcpcd) over bonding has worked for years, although I've not
personally tried it recently.  Perhaps it varies by distro.  In any
event, this behavior of bonding (setting the bond's MAC without a
down/up flip) has never been different in my memory.

	I've not yet dug down to see if NETDEV_CHANGEADDR will result in
an RTM_NEWLINK to user space.  At first glance it doesn't look like it.

	When bonding goes link up, however, I think linkwatch_do_dev
will issue an RTM_NEWLINK (via a call to netdev_state_change), or,
alternately, dev_change_flags will do it at IFF_UP time.

	-J

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

Re: The bonding driver should notify userspace of MAC address change

[Nicolas de Pesloüan]

Le 15/04/2011 23:45, Jay Vosburgh a écrit :
> Nicolas de Pesloüan<nicolas.2p.debian@gmail.com>  wrote:
>
>> Agreed.
>>
>>> 	Is there some race window there between the register and the
>>> netif_carrier_off?
>>
>> It might be that dhcpd does not wait for link to be up before starting to send DHCP requests.
>
> 	It looks like it's not related to carrier state at all:
>
> #212: dhcpcd requires restart to get an IP address for bonded interface
> -----------------------+-----------------
>    Reporter:  mgorny@…  |      Owner:  roy
>        Type:  defect    |     Status:  new
>    Priority:  major     |  Milestone:
>   Component:  dhcpcd    |    Version:  5.1
> Resolution:            |   Keywords:
> -----------------------+-----------------
>
> Comment (by roy):
>
>   Sorry, the above isn't too clear.
>
>   dhcpcd will read the hardware address when the interface is marked IFF_UP
>   or when given RTM_NEWLINK with ifi->ifi_change = ~0U, the latter being
>   sent by some drivers to tell userland that an interface characteristic has
>   changed - like say a hardware address - if the driver supports such a
>   change whilst still up. Normal behaviour is to mark device as DOWN before
>   changing hardware address. bonding does this whilst marked UP, hence this
>   issue.
>
>   carrier going up / down is just that, it's not a signal to re-read the
>   interface characteristics.
>
>
> 	Now this confuses me again; I thought that running the dhcp
> client (dhcpcd) over bonding has worked for years, although I've not
> personally tried it recently.  Perhaps it varies by distro.  In any
> event, this behavior of bonding (setting the bond's MAC without a
> down/up flip) has never been different in my memory.

I use dhcpcd over bonding everyday on my laptop (debian/testing), for several years.

bond0 uses eth0 and wlan0 as slaves. The time to get a DHCP reply vary, mostly because the time to 
register to the wifi AP is not perfectly stable. Sometimes (not very often), I need to ifdown/ifup 
to get a DHCP reply. This might be due to dhcpcd reading the MAC too early and getting an all zero 
MAC. I need to try and double check this, but as it happen early in the boot process and not very 
often, it will be hard to diagnose.

That being said, sending a DHCP request with an all zero source MAC sounds very strange to me.

The RFC for DHCP (RFC2131) states that a DHCP server must be prepared to broadcast the reply if the 
client hardware address is null.

    If 'giaddr' is zero and 'ciaddr' is zero, and the broadcast bit is
    set, then the server broadcasts DHCPOFFER and DHCPACK messages to
    0xffffffff. If the broadcast bit is not set and 'giaddr' is zero and
    'ciaddr' is zero, then the server unicasts DHCPOFFER and DHCPACK
    messages to the client's hardware address and 'yiaddr' address.  In
    all cases, when 'giaddr' is zero, the server broadcasts any DHCPNAK
    messages to 0xffffffff.

But a DHCP server must store a client identifier associated with a given dynamic IP address for the 
duration of the lease, to be able to give the same IP address to the same client if this client 
restarts while the lease is still valid.

If the client hardware address is all zero and the client configuration does not provide some other 
user defined identifier, then the client identifier is not unique and the DHCP server should - I 
think - ignore the request.

So, except if the local dhcpcd configuration contains an user defined identifier, I think dhcpcd 
should wait for the MAC address to be not null before sending any requests.

	Nicolas.

> 	I've not yet dug down to see if NETDEV_CHANGEADDR will result in
> an RTM_NEWLINK to user space.  At first glance it doesn't look like it.
>
> 	When bonding goes link up, however, I think linkwatch_do_dev
> will issue an RTM_NEWLINK (via a call to netdev_state_change), or,
> alternately, dev_change_flags will do it at IFF_UP time.
>
> 	-J
>
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

Re: [RFC net-next] bonding: notify when bonding device address changes

[Stephen Hemminger]

On Fri, 15 Apr 2011 12:28:24 -0700
Jay Vosburgh <fubar@us.ibm.com> wrote:

> Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
> >When a device changes its hardware address, it needs to call the network
> >device notifiers to inform protocols.
> >
> >Compile tested only.

I did not audit that all the call sites have the proper locking.
When calling notifier, no bridge locks should be held and RTNL mutex
should be held.