[PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[David Shwatrz]

Hello,

Attached here is the patch - please read the following text here below:

 We know that changes in MAC addresses are not frequent but
we are working on a special, highly advanced networking Linux based project
in our LABs, where we do change MAC addresses of an interface quite frequently.
(We do not go totally wild; the MAC addresses we are changing into are
from a set of given MAC addresses).

Normally, when we change a MAC address of some interface, the
relevant neighbours in the LAN which have entries with the previous MAC
address are not sent any update notification; instead, it is there
regular timers mechanisms which update the MAC address to the new
one in their ARP tables.

I had written a small patch to neigh_changeaddr() in net/core/neighbour.c
against the 2.6 git net tree, which sends a gratuitous ARP to update
the list of
all the involved neighbours with the change of MAC address.
The patch is for neigh_changeaddr() only.

This patch was tested and it does work in my LAB; if such a patch is
not needed,
I wonder why ?
It seems to me that it could not cause any troubles.
BTW, I had noticed that in irlan driver, there is such a mechanism
of sending a gratuitous ARP to update all the
neighbours when a MAC address is changed.

Signed-off-by: David Shwatrz <dshwatrz@gmail.com>

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[YOSHIFUJI Hideaki / 吉藤英明]

In article <31436f4a0712230341x5a0abf08q7e6f7917ba7e38f0@mail.gmail.com> (at Sun, 23 Dec 2007 13:41:36 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:

> I had written a small patch to neigh_changeaddr() in net/core/neighbour.c
> against the 2.6 git net tree, which sends a gratuitous ARP to update
> the list of
> all the involved neighbours with the change of MAC address.
> The patch is for neigh_changeaddr() only.

Though I can see no patch, but I disagree. ;-)
I do think you should change arp_netdev_event() and ndisc_netdev_event().

--yoshfuji

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[David Shwatrz]

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

Hi,
Oop, I am TWICE sorry ! I wrongly attached a wrong, empty file.
Attached here is the patch.

Regarding your answer;  I accept it and I will soon send a revised
version of this patch (making changes to
 arp_netdev_event() and ndisc_netdev_event().)
I had  IPv4 in mind, there is no reason that it will no be also in IPv6.

Regads,
David Shwatrz



On Dec 23, 2007 2:11 PM, YOSHIFUJI Hideaki / 吉藤英明
<yoshfuji@linux-ipv6.org> wrote:
> In article <31436f4a0712230341x5a0abf08q7e6f7917ba7e38f0@mail.gmail.com> (at Sun, 23 Dec 2007 13:41:36 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:
>
> > I had written a small patch to neigh_changeaddr() in net/core/neighbour.c
> > against the 2.6 git net tree, which sends a gratuitous ARP to update
> > the list of
> > all the involved neighbours with the change of MAC address.
> > The patch is for neigh_changeaddr() only.
>
> Though I can see no patch, but I disagree. ;-)
> I do think you should change arp_netdev_event() and ndisc_netdev_event().
>
> --yoshfuji
>

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1070 bytes --]

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 29b8ee4..ddeae82 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -36,6 +36,9 @@
 #include <linux/string.h>
 #include <linux/log2.h>
 
+#include <linux/inetdevice.h>
+#include <net/arp.h>
+
 #define NEIGH_DEBUG 1
 
 #define NEIGH_PRINTK(x...) printk(x)
@@ -228,9 +231,26 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
 
 void neigh_changeaddr(struct neigh_table *tbl, struct net_device *dev)
 {
+	struct in_device *in_dev;
 	write_lock_bh(&tbl->lock);
 	neigh_flush_dev(tbl, dev);
 	write_unlock_bh(&tbl->lock);
+	 
+	/* Send a gratuitous ARP to the neighbours to update their arp tables */
+	
+	rcu_read_lock();
+	in_dev = __in_dev_get_rcu(dev);
+	if (in_dev == NULL)
+		goto out;
+	if (in_dev->ifa_list)
+		
+	arp_send(ARPOP_REQUEST, ETH_P_ARP, 
+		 in_dev->ifa_list->ifa_address,
+		 dev, 
+		 in_dev->ifa_list->ifa_address,
+		 NULL, dev->dev_addr, NULL);
+out:
+	rcu_read_unlock();
 }
 
 int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[YOSHIFUJI Hideaki / 吉藤英明]

In article <31436f4a0712230424n1cfaeb27o463e62093af41090@mail.gmail.com> (at Sun, 23 Dec 2007 14:24:00 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:

> Regarding your answer;  I accept it and I will soon send a revised
> version of this patch (making changes to
>  arp_netdev_event() and ndisc_netdev_event().)
> I had  IPv4 in mind, there is no reason that it will no be also in IPv6.

You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
For IPv6, we also have anycast (maintained by ac_list) as well.

--yoshfuji

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[jamal]

On Sun, 2007-23-12 at 21:38 +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <31436f4a0712230424n1cfaeb27o463e62093af41090@mail.gmail.com> (at Sun, 23 Dec 2007 14:24:00 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:
> 
> > Regarding your answer;  I accept it and I will soon send a revised
> > version of this patch (making changes to
> >  arp_netdev_event() and ndisc_netdev_event().)
> > I had  IPv4 in mind, there is no reason that it will no be also in IPv6.
> 
> You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> For IPv6, we also have anycast (maintained by ac_list) as well.
> 

Hrm, how is this going to work for the case of multiple MACs on a
device? 
Changing one MAC address doesnt equate to issuing a grat arp with _the
new MAC_ for all ifa (given each MAC may be map to a different ifa) 

cheers,
jamal

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1198413975.4423.65.camel@localhost> (at Sun, 23 Dec 2007 07:46:15 -0500), jamal <hadi@cyberus.ca> says:

> On Sun, 2007-23-12 at 21:38 +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > In article <31436f4a0712230424n1cfaeb27o463e62093af41090@mail.gmail.com> (at Sun, 23 Dec 2007 14:24:00 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:
> > 
> > > Regarding your answer;  I accept it and I will soon send a revised
> > > version of this patch (making changes to
> > >  arp_netdev_event() and ndisc_netdev_event().)
> > > I had  IPv4 in mind, there is no reason that it will no be also in IPv6.
> > 
> > You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> > For IPv6, we also have anycast (maintained by ac_list) as well.
> > 
> 
> Hrm, how is this going to work for the case of multiple MACs on a
> device? 
> Changing one MAC address doesnt equate to issuing a grat arp with _the
> new MAC_ for all ifa (given each MAC may be map to a different ifa) 

If the secondary MACs are used with ARP/NDP, we should take care of
that, but I think we use the primary MAC for ARP/NDP, no?
(In other words, we always use primary MAC for ARP reply / NA, no?)

--yoshfuji

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[jamal]

On Sun, 2007-23-12 at 22:04 +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:

> If the secondary MACs are used with ARP/NDP, we should take care of
> that, but I think we use the primary MAC for ARP/NDP, no?
> (In other words, we always use primary MAC for ARP reply / NA, no?)

I think it maybe a policy decision; 
In the IPV4 case, where the system owns the IP addresses for the
classical scenario where there is a single MAC address per ethx then we
always respond with MAC address of ethx wherever the arp request was
received from. I think it is different in the case of IPV6 where the eth
device owns the IP address, no? i.e is that where you are drawing the
concept of primary MAC?
The case of multiple MACs per interface requires further policy
resolution IMO. It would be nice to be able to tell the kernel which MAC
to use when responding for which ip address it owns. This can be then
easily mapped to the routing table src address selection.
Patrick?

cheers,
jamal

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[jamal]

On Sun, 2007-23-12 at 08:17 -0500, jamal wrote:
> On Sun, 2007-23-12 at 22:04 +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> 
> > If the secondary MACs are used with ARP/NDP, we should take care of
> > that, but I think we use the primary MAC for ARP/NDP, no?
> > (In other words, we always use primary MAC for ARP reply / NA, no?)
> 
> I think it maybe a policy decision; 

Never mind, that was my body being in a different time zone.
I went back and looked at a little chat i had with Patrick when he
posted with the macvlan driver.
At the moment we are still maintaining the model that a NIC can _only_
appear to have a single MAC to the upper layers. IOW, you have to
instantiate a macvlan device for each of the 16 MAC addresses on the
e1000 if you need to expose them. This means that the ip layer - even
with multiple ip addresses will only ever see one MAC address. 
Note also: The name macvlan is a little misleading since it allows for
the above without need for vlans.

cheers,
jamal

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[David Shwatrz]

Hello,


>You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> For IPv6, we also have anycast (maintained by ac_list) as well.

I am not sure that we need to iterate all of ifa_list in IPv4.
The reason is that we end with arp_send, and it initiates a broadcast.
So all neighbours will receive it and update their arp tables
accordingly.
The dest hw in the arp_send is NULL according to this patch ; this means that
we will assign dev->broadcast to dest_hw  in apr_create().

It seems to me there's no reason to send more than one broadcast.

In IPv6, I need to check, since it is multicast.

Thoughts ?

Regards,
DS





On Dec 23, 2007 2:38 PM, YOSHIFUJI Hideaki / 吉藤英明
<yoshfuji@linux-ipv6.org> wrote:
> In article <31436f4a0712230424n1cfaeb27o463e62093af41090@mail.gmail.com> (at Sun, 23 Dec 2007 14:24:00 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:
>
> > Regarding your answer;  I accept it and I will soon send a revised
> > version of this patch (making changes to
> >  arp_netdev_event() and ndisc_netdev_event().)
> > I had  IPv4 in mind, there is no reason that it will no be also in IPv6.
>
> You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> For IPv6, we also have anycast (maintained by ac_list) as well.
>
> --yoshfuji
>
>

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[YOSHIFUJI Hideaki / 吉藤英明]

In article <31436f4a0712230504x7b0b4f38i8f69a7a825bcc14b@mail.gmail.com> (at Sun, 23 Dec 2007 15:04:37 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:

> Hello,
> 
> 
> >You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> > For IPv6, we also have anycast (maintained by ac_list) as well.
> 
> I am not sure that we need to iterate all of ifa_list in IPv4.
> The reason is that we end with arp_send, and it initiates a broadcast.
> So all neighbours will receive it and update their arp tables
> accordingly.
> The dest hw in the arp_send is NULL according to this patch ; this means that
> we will assign dev->broadcast to dest_hw  in apr_create().
> 
> It seems to me there's no reason to send more than one broadcast.

Urgh? what is happend if you have multiple IPv4 addresses on the device?


> In IPv6, I need to check, since it is multicast.

Please read RFC2461 Section 7.2.6.  In short we should send a few
unsolicited NA, but I think you can start from sending once per an
address.

--yoshfuji

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[David Shwatrz]

Yoshfuji,

Thanks, you are right ! soon I will send the patches.

Thanks !
DS


On Dec 23, 2007 3:13 PM, YOSHIFUJI Hideaki / 吉藤英明
<yoshfuji@linux-ipv6.org> wrote:
> In article <31436f4a0712230504x7b0b4f38i8f69a7a825bcc14b@mail.gmail.com> (at Sun, 23 Dec 2007 15:04:37 +0200), "David Shwatrz" <dshwatrz@gmail.com> says:
>
> > Hello,
> >
> >
> > >You should iterate all of ifa_list (for IPv4) / addr_list (for IPv6).
> > > For IPv6, we also have anycast (maintained by ac_list) as well.
> >
> > I am not sure that we need to iterate all of ifa_list in IPv4.
> > The reason is that we end with arp_send, and it initiates a broadcast.
> > So all neighbours will receive it and update their arp tables
> > accordingly.
> > The dest hw in the arp_send is NULL according to this patch ; this means that
> > we will assign dev->broadcast to dest_hw  in apr_create().
> >
> > It seems to me there's no reason to send more than one broadcast.
>
> Urgh? what is happend if you have multiple IPv4 addresses on the device?
>
>
> > In IPv6, I need to check, since it is multicast.
>
> Please read RFC2461 Section 7.2.6.  In short we should send a few
> unsolicited NA, but I think you can start from sending once per an
> address.
>
> --yoshfuji
>

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[Herbert Xu]

David Shwatrz <dshwatrz@gmail.com> wrote:
> 
> Hi,
> Oop, I am TWICE sorry ! I wrongly attached a wrong, empty file.
> Attached here is the patch.
> 
> Regarding your answer;  I accept it and I will soon send a revised
> version of this patch (making changes to
> arp_netdev_event() and ndisc_netdev_event().)
> I had  IPv4 in mind, there is no reason that it will no be also in IPv6.

Hmm, why can't you do this from user-space?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[David Shwatrz]

Hello,

First, it indeed can be handled by user space. (even though it should
be done twice, once for ifconig of net-tools  and once for ip of
iproute2) / However, we have already
methods which deal with bringing down an interface - neigh_ifdown(),
and changing MAC address of an interface (neigh_changeaddr). So why
not do it from
the kernel ?
DS

On Dec 23, 2007 4:02 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> David Shwatrz <dshwatrz@gmail.com> wrote:
> >
> > Hi,
> > Oop, I am TWICE sorry ! I wrongly attached a wrong, empty file.
> > Attached here is the patch.
> >
> > Regarding your answer;  I accept it and I will soon send a revised
> > version of this patch (making changes to
> > arp_netdev_event() and ndisc_netdev_event().)
> > I had  IPv4 in mind, there is no reason that it will no be also in IPv6.
>
> Hmm, why can't you do this from user-space?
>
> Cheers,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[jamal]

On Mon, 2007-24-12 at 15:38 +0200, David Shwatrz wrote:
> Hello,
> 
> First, it indeed can be handled by user space. (even though it should
> be done twice, once for ifconig of net-tools  and once for ip of
> iproute2) 

it needs to be done once only: reacting to netlink events when MAC
address changes.

> / However, we have already
> methods which deal with bringing down an interface - neigh_ifdown(),
> and changing MAC address of an interface (neigh_changeaddr). So why
> not do it from the kernel ?

Herbert, i agree with you that userspace is the best spot for this[1];
we unfortunately have precedence already on the kernel sending arps with
bonding when link status changes (that was added recently).
So it sounds reasonable to have this patch in the kernel as well.

cheers,
jamal

[1] Things like these tend to be very policy rich and thats why user
space is the best spot for them.
I have infact implemented this feature in user space in some random box
i have where i failover MACs for HA reasons. Depending on how much
traffic there is on the wire, arps do get dropped.
One of the hardest things to decide on was how many times to retry the
grat arp sending and what the timeout would be between each sent
gratarp. 
The earlier patch posted didnt consider this but would be nice to have a
couple of sysctls to add the two parameters if this makes it in.

Re: [PATCH net-2.6][NEIGH] Updating affected neighbours when about MAC address change

[Herbert Xu]

On Mon, Dec 24, 2007 at 09:50:59AM -0500, jamal wrote:
>
> Herbert, i agree with you that userspace is the best spot for this[1];
> we unfortunately have precedence already on the kernel sending arps with
> bonding when link status changes (that was added recently).
> So it sounds reasonable to have this patch in the kernel as well.

Well just because we've made a mistake in the past does not mean
that we should repeat it over and over again.  If we keep doing
this the kernel will soon end up as a whole heap of junk.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt