IFF_NOARP & broadcasting

IFF_NOARP & broadcasting

[Karlis Peisenieks]

Hello!

Is there any serious reason why when device has IFF_NOARP set, devices ll
address is used as destination address even for broadcasts (and even when
devices broadcast address is valid)? Currently it is impossible to send out
correct broadcast over ethernet when IFF_NOARP is set.

The code in question is in net/ipv4/arp.c, function arp_constructor:

                if (neigh->type == RTN_MULTICAST) {
                        neigh->nud_state = NUD_NOARP;
                        arp_mc_map(addr, neigh->ha, dev, 1);
                } else if (dev->flags&(IFF_NOARP|IFF_LOOPBACK)) {
                                       ^^^^^^^^^
                        neigh->nud_state = NUD_NOARP;
                        memcpy(neigh->ha, dev->dev_addr, dev->addr_len);
                } else if (neigh->type == RTN_BROADCAST ||
dev->flags&IFF_POINT neigh->nud_state = NUD_NOARP;
                        memcpy(neigh->ha, dev->broadcast, dev->addr_len);
                }

Is it ok to apply following patch and not break anything:

--- arp.c.orig	Wed Jul 24 19:02:53 2002
+++ arp.c	Wed Jul 24 19:01:06 2002
@@ -289,12 +289,15 @@
 		if (neigh->type == RTN_MULTICAST) {
 			neigh->nud_state = NUD_NOARP;
 			arp_mc_map(addr, neigh->ha, dev, 1);
-		} else if (dev->flags&(IFF_NOARP|IFF_LOOPBACK)) {
+		} else if (dev->flags&IFF_LOOPBACK) {
 			neigh->nud_state = NUD_NOARP;
 			memcpy(neigh->ha, dev->dev_addr, dev->addr_len);
 		} else if (neigh->type == RTN_BROADCAST ||
dev->flags&IFF_POINTOPOINT) {
 			neigh->nud_state = NUD_NOARP;
 			memcpy(neigh->ha, dev->broadcast, dev->addr_len);
+		} else if (dev->flags & IFF_NOARP) {
+			neigh->nud_state = NUD_NOARP;
+			memcpy(neigh->ha, dev->dev_addr, dev->addr_len);
 		}
 		if (dev->hard_header_cache)
 			neigh->ops = &arp_hh_ops;

Re: IFF_NOARP & broadcasting

[kuznet]

Hello!

> Is there any serious reason why when device has IFF_NOARP set, devices ll
> address is used as destination address even for broadcasts (and even when
> devices broadcast address is valid)?

No reasons, I think. Instead of that your patch, it may be better just 
exchange order of tests. As I remember, ll addr is initialized
to dev_addr for NOARP just to get a valid unicast address as destination
for eth_type_trans() to work correctly.


> Currently it is impossible to send out correct broadcast over ethernet
> when IFF_NOARP is set.

No, it is surely not impossible. f.e.

ip neigh add 255.255.255.255 dev dummy0 lladdr ff:ff:ff:ff:ff:ff

works.

Alexey

Re: IFF_NOARP & broadcasting

[Karlis Peisenieks]

On Wed, 24 Jul 2002 kuznet@ms2.inr.ac.ru wrote:

> > Is there any serious reason why when device has IFF_NOARP set, devices ll
> > address is used as destination address even for broadcasts (and even when
> > devices broadcast address is valid)?
> 
> No reasons, I think. Instead of that your patch, it may be better just 
> exchange order of tests. As I remember, ll addr is initialized

Ok, please consider the attached patch.

> No, it is surely not impossible. f.e.
> 
> ip neigh add 255.255.255.255 dev dummy0 lladdr ff:ff:ff:ff:ff:ff

Umm, did not think of this. Anyway, this forces to add one neigh for each 
broadcast address in use on interface.

Here comes the patch:

--- arp.c.orig	Wed Jul 24 20:43:13 2002
+++ arp.c	Wed Jul 24 20:42:53 2002
@@ -289,12 +289,12 @@
 		if (neigh->type == RTN_MULTICAST) {
 			neigh->nud_state = NUD_NOARP;
 			arp_mc_map(addr, neigh->ha, dev, 1);
-		} else if (dev->flags&(IFF_NOARP|IFF_LOOPBACK)) {
-			neigh->nud_state = NUD_NOARP;
-			memcpy(neigh->ha, dev->dev_addr, dev->addr_len);
 		} else if (neigh->type == RTN_BROADCAST || dev->flags&IFF_POINTOPOINT) {
 			neigh->nud_state = NUD_NOARP;
 			memcpy(neigh->ha, dev->broadcast, dev->addr_len);
+		} else if (dev->flags&(IFF_NOARP|IFF_LOOPBACK)) {
+			neigh->nud_state = NUD_NOARP;
+			memcpy(neigh->ha, dev->dev_addr, dev->addr_len);
 		}
 		if (dev->hard_header_cache)
 			neigh->ops = &arp_hh_ops;



Karlis

Re: IFF_NOARP & broadcasting [2]

[Karlis Peisenieks]

On 2002.07.24 19:52 kuznet@ms2.inr.ac.ru wrote:

> No, it is surely not impossible. f.e.
> 
> ip neigh add 255.255.255.255 dev dummy0 lladdr ff:ff:ff:ff:ff:ff

Ok, but even in this case there is minor problem. And it lies in
net/ethernet/eth.c - eth_header.

neigh_resolve_output creates header cache with correct ll source and ll
destination as in neighbour, but uses dev->hard_header to build header for
first packet. And eth_header has code:

        if (dev->flags & (IFF_LOOPBACK|IFF_NOARP)) 
        {
                memset(eth->h_dest, 0, dev->addr_len);
                return(dev->hard_header_len);
        }

which explicitly sets ll destination to all 0s. For all next packets
hh_cache is used and they get correct ll address (as in neighbour).

Please comment on this patch :) :

--- eth.c.orig	Thu Jul 25 16:16:37 2002
+++ eth.c	Thu Jul 25 16:16:47 2002
@@ -100,7 +100,7 @@
 	 *	Anyway, the loopback-device should never use this
function... 
 	 */
 
-	if (dev->flags & (IFF_LOOPBACK|IFF_NOARP)) 
+	if (dev->flags & IFF_LOOPBACK) 
 	{
 		memset(eth->h_dest, 0, dev->addr_len);
 		return(dev->hard_header_len);



Karlis

Re: IFF_NOARP & broadcasting [2]

[kuznet]

Hello!

> Ok, but even in this case there is minor problem. And it lies in
> net/ethernet/eth.c - eth_header.

You have just made great discovery. :-)

It was big puzzle why people blame that they see packets with zero MAC,
sometimes. Apparently, this happens on the first packet, when hh cache
is still not created. :-)


> Please comment on this patch :) :

I am not sure that this is right. Protocols using neighbour cache
will work, but anothers will fail on NOARP devices.

It is safer just to reorder two ifs, checking for daddr!=NULL first.

Alexey

Re: IFF_NOARP & broadcasting [2]

[Karlis Peisenieks]

On 2002.07.25 17:02 kuznet@ms2.inr.ac.ru wrote:

> You have just made great discovery. :-)
> 
> It was big puzzle why people blame that they see packets with zero MAC,
> sometimes. Apparently, this happens on the first packet, when hh cache
> is still not created. :-)

Yes, this was killing me too.

> I am not sure that this is right. Protocols using neighbour cache
> will work, but anothers will fail on NOARP devices.
> 
> It is safer just to reorder two ifs, checking for daddr!=NULL first.

You mean like this:

--- eth.c.orig	Thu Jul 25 17:50:20 2002
+++ eth.c	Thu Jul 25 17:50:35 2002
@@ -96,6 +96,12 @@
 	else
 		memcpy(eth->h_source,dev->dev_addr,dev->addr_len);
 
+	if(daddr)
+	{
+		memcpy(eth->h_dest,daddr,dev->addr_len);
+		return dev->hard_header_len;
+	}
+	
 	/*
 	 *	Anyway, the loopback-device should never use this
function... 
 	 */
@@ -106,12 +112,6 @@
 		return(dev->hard_header_len);
 	}
 	
-	if(daddr)
-	{
-		memcpy(eth->h_dest,daddr,dev->addr_len);
-		return dev->hard_header_len;
-	}
-	
 	return -dev->hard_header_len;
 }
 

Karlis