Patchwork [RFC,net-next] ipv4: use bcast as dst address in case IFF_NOARP is set

login
register
mail settings
Submitter Jiri Pirko
Date Jan. 8, 2013, 1:02 p.m.
Message ID <1357650162-5554-1-git-send-email-jiri@resnulli.us>
Download mbox | patch
Permalink /patch/210368/
State RFC
Delegated to: David Miller
Headers show

Comments

Jiri Pirko - Jan. 8, 2013, 1:02 p.m.
When IFF_NOARP is set on a device, dev->dev_addr is used as *dst*
addr of sent frames. That does not make sense. Use rather bcast address
instead.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/ipv4/arp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Stephen Hemminger - Jan. 8, 2013, 5:11 p.m.
> When IFF_NOARP is set on a device, dev->dev_addr is used as *dst*
> addr of sent frames. That does not make sense. Use rather bcast
> address
> instead.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

What did you test this on? I think this may have been
intentional to avoid broadcasting.
--
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
Jiri Pirko - Jan. 8, 2013, 5:45 p.m.
Tue, Jan 08, 2013 at 06:11:55PM CET, stephen.hemminger@vyatta.com wrote:
>
>> When IFF_NOARP is set on a device, dev->dev_addr is used as *dst*
>> addr of sent frames. That does not make sense. Use rather bcast
>> address
>> instead.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
>What did you test this on? I think this may have been
>intentional to avoid broadcasting.

Thanks for looking at this Stephen.

I tested this on two boxes connected via ethernet.

I believe this may have been intentional, but what sense does it have to
use dev_addr as destination address? That is what I do not understand.

Also, what is the issue with sending all packets to broadcast when NOARP is
set? In my opinion, it only makes sense.
--
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
Stephen Hemminger - Jan. 8, 2013, 7:10 p.m.
> Tue, Jan 08, 2013 at 06:11:55PM CET, stephen.hemminger@vyatta.com
> wrote:
> >
> >> When IFF_NOARP is set on a device, dev->dev_addr is used as *dst*
> >> addr of sent frames. That does not make sense. Use rather bcast
> >> address
> >> instead.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >
> >What did you test this on? I think this may have been
> >intentional to avoid broadcasting.
> 
> Thanks for looking at this Stephen.
> 
> I tested this on two boxes connected via ethernet.
> 
> I believe this may have been intentional, but what sense does it have
> to
> use dev_addr as destination address? That is what I do not
> understand.
> 
> Also, what is the issue with sending all packets to broadcast when
> NOARP is
> set? In my opinion, it only makes sense.
> 

It looks like NOARP devices are point-to-point and don't really
have any concept of broadcast.
--
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
Jan Engelhardt - Jan. 8, 2013, 9:51 p.m.
On Tuesday 2013-01-08 20:10, Stephen Hemminger wrote:
>> 
>>Also, what is the issue with sending all packets to broadcast when
>>NOARP is set? In my opinion, it only makes sense.
>
>It looks like NOARP devices are point-to-point and don't really have
>any concept of broadcast.

PTP links usually are NOARP, but NOARP does not necessarily imply
a PTP link.
--
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
David Miller - Jan. 8, 2013, 10:27 p.m.
From: Jiri Pirko <jiri@resnulli.us>
Date: Tue,  8 Jan 2013 14:02:42 +0100

> When IFF_NOARP is set on a device, dev->dev_addr is used as *dst*
> addr of sent frames. That does not make sense. Use rather bcast address
> instead.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

The address is actually arbitrary in this case, I believe the
current behavior is intentional, and even if it is not intentional
since the address is arbitrary this change carrys only potential
risk for zero gain.
--
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
David Miller - Jan. 8, 2013, 10:33 p.m.
From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 8 Jan 2013 18:45:38 +0100

> I believe this may have been intentional, but what sense does it have to
> use dev_addr as destination address? That is what I do not understand.

It's the only address we know isn't someone else's and is not broadcast.
--
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

Patch

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 9547a273..19d5ac2 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -290,11 +290,11 @@  static int arp_constructor(struct neighbour *neigh)
 		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)) {
+			   (dev->flags & (IFF_POINTOPOINT | IFF_NOARP))) {
 			neigh->nud_state = NUD_NOARP;
 			memcpy(neigh->ha, dev->broadcast, dev->addr_len);
 		}