| Message ID | 201001050004.45004.opurdila@ixiacom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Reviewed-by: Laurent Chavey <chavey@google.com> On Mon, Jan 4, 2010 at 2:04 PM, Octavian Purdila <opurdila@ixiacom.com> wrote: > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > --- > > I've noticed that even though we currently support response type gratuitous ARP > [response type, source mac, dest mac, source IP, source IP] *with a clean ARP table* > we do not support the request type [request type, source mac, ff:ff:ff:ff:ff:ff, source IP, source IP]. > > This patch makes request type work as well, but RFC2002 says that gratuitous ARP > (both request and response) must update the ARP table *if* the IP already > exists in the table: > > In either case, for a gratuitous ARP, the ARP packet MUST be > transmitted as a local broadcast packet on the local link. As > specified in [16], any node receiving any ARP packet (Request or > Reply) MUST update its local ARP cache with the Sender Protocol > and Hardware Addresses in the ARP packet, if the receiving node > has an entry for that IP address already in its ARP cache. This > requirement in the ARP protocol applies even for ARP Request > packets, and for ARP Reply packets that do not match any ARP > Request transmitted by the receiving node [16]. > > so, I am not sure if this is right. But current behavior for response type > gratuitous ARP does not seem to be covered by the RFC either. > > net/ipv4/arp.c | 11 +++++++++-- > 1 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c > index c95cd93..81ef2d5 100644 > --- a/net/ipv4/arp.c > +++ b/net/ipv4/arp.c > @@ -811,8 +811,13 @@ static int arp_process(struct sk_buff *skb) > goto out; > } > > - if (arp->ar_op == htons(ARPOP_REQUEST) && > - ip_route_input(skb, tip, sip, 0, dev) == 0) { > + if (arp->ar_op == htons(ARPOP_REQUEST)) { > + /* gratuitous ARP */ > + if (tip == sip) { > + n = neigh_event_ns(&arp_tbl, sha, &sip, dev); > + goto update; > + } else if (ip_route_input(skb, tip, sip, 0, dev) != 0) > + goto update_lookup; > > rt = skb_rtable(skb); > addr_type = rt->rt_type; > @@ -853,6 +858,7 @@ static int arp_process(struct sk_buff *skb) > } > } > > +update_lookup: > /* Update our ARP tables */ > > n = __neigh_lookup(&arp_tbl, &sip, dev, 0); > @@ -868,6 +874,7 @@ static int arp_process(struct sk_buff *skb) > n = __neigh_lookup(&arp_tbl, &sip, dev, 1); > } > > +update: > if (n) { > int state = NUD_REACHABLE; > int override; > -- > 1.5.6.5 > -- > 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 > -- 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
From: Octavian Purdila <opurdila@ixiacom.com> Date: Tue, 5 Jan 2010 00:04:44 +0200 > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > --- > > I've noticed that even though we currently support response type gratuitous ARP > [response type, source mac, dest mac, source IP, source IP] *with a clean ARP table* > we do not support the request type [request type, source mac, ff:ff:ff:ff:ff:ff, source IP, source IP]. Please don't submit your patches in this manner. All of these relevant, interesting, details belong in the commit message. But any text you place aftr the "---" line will be omitted from the commit message when your patch is applied by automated GIT tools. I've done some research and I'm happy to apply your patch to net-next-2.6 once it is submitted properly. In fact we need to do some more research in this area because generally we should more mimick the processing order of ARP prescribed in the RFC. In particular we should test the operation code lastly, which would avoid these kinds of inconsistencies. I'm worried though about security issues as well, as we make more the acceptance more and more liberal, it becomes that much easier for machines on the local network to poison ARP entries and use that to either accept all traffic destined for a particular node or simply deny that node access to the network. In particular the kernel currently explicitly does not accept unsolicited ARP, and this is controlled by the ARP_ACCEPT per-device option. if (IPV4_DEVCONF_ALL(dev_net(dev), ARP_ACCEPT)) { /* Unsolicited ARP is not accepted by default. It is possible, that this option should be enabled for some devices (strip is candidate) */ ... -- 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
On Sunday 10 January 2010 23:21:05 you wrote: > From: Octavian Purdila <opurdila@ixiacom.com> > Date: Tue, 5 Jan 2010 00:04:44 +0200 > > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > > --- > > > > I've noticed that even though we currently support response type > > gratuitous ARP [response type, source mac, dest mac, source IP, source > > IP] *with a clean ARP table* we do not support the request type [request > > type, source mac, ff:ff:ff:ff:ff:ff, source IP, source IP]. > > Please don't submit your patches in this manner. > > All of these relevant, interesting, details belong in the commit > message. But any text you place aftr the "---" line will be omitted > from the commit message when your patch is applied by automated GIT > tools. > Got it. > I've done some research and I'm happy to apply your patch to > net-next-2.6 once it is submitted properly. > Thanks, I'll resubmitted it properly in a bit. > In fact we need to do some more research in this area because > generally we should more mimick the processing order of ARP prescribed > in the RFC. In particular we should test the operation code lastly, > which would avoid these kinds of inconsistencies. > > I'm worried though about security issues as well, as we make more the > acceptance more and more liberal, it becomes that much easier for > machines on the local network to poison ARP entries and use that to > either accept all traffic destined for a particular node or simply > deny that node access to the network. > > In particular the kernel currently explicitly does not accept > unsolicited ARP, and this is controlled by the ARP_ACCEPT per-device > option. > > if (IPV4_DEVCONF_ALL(dev_net(dev), ARP_ACCEPT)) { > /* Unsolicited ARP is not accepted by default. > It is possible, that this option should be enabled for some > devices (strip is candidate) > */ > ... > Good point, I'll try to respin the patch to do this check for the request type gratuitous ARP as well. Thanks! -- 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
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index c95cd93..81ef2d5 100644 --- a/net/ipv4/arp.c +++ b/net/ipv4/arp.c @@ -811,8 +811,13 @@ static int arp_process(struct sk_buff *skb) goto out; } - if (arp->ar_op == htons(ARPOP_REQUEST) && - ip_route_input(skb, tip, sip, 0, dev) == 0) { + if (arp->ar_op == htons(ARPOP_REQUEST)) { + /* gratuitous ARP */ + if (tip == sip) { + n = neigh_event_ns(&arp_tbl, sha, &sip, dev); + goto update; + } else if (ip_route_input(skb, tip, sip, 0, dev) != 0) + goto update_lookup; rt = skb_rtable(skb); addr_type = rt->rt_type; @@ -853,6 +858,7 @@ static int arp_process(struct sk_buff *skb) } } +update_lookup: /* Update our ARP tables */ n = __neigh_lookup(&arp_tbl, &sip, dev, 0); @@ -868,6 +874,7 @@ static int arp_process(struct sk_buff *skb) n = __neigh_lookup(&arp_tbl, &sip, dev, 1); } +update: if (n) { int state = NUD_REACHABLE; int override;
Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> --- I've noticed that even though we currently support response type gratuitous ARP [response type, source mac, dest mac, source IP, source IP] *with a clean ARP table* we do not support the request type [request type, source mac, ff:ff:ff:ff:ff:ff, source IP, source IP]. This patch makes request type work as well, but RFC2002 says that gratuitous ARP (both request and response) must update the ARP table *if* the IP already exists in the table: In either case, for a gratuitous ARP, the ARP packet MUST be transmitted as a local broadcast packet on the local link. As specified in [16], any node receiving any ARP packet (Request or Reply) MUST update its local ARP cache with the Sender Protocol and Hardware Addresses in the ARP packet, if the receiving node has an entry for that IP address already in its ARP cache. This requirement in the ARP protocol applies even for ARP Request packets, and for ARP Reply packets that do not match any ARP Request transmitted by the receiving node [16]. so, I am not sure if this is right. But current behavior for response type gratuitous ARP does not seem to be covered by the RFC either. net/ipv4/arp.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)