Change proxy_arp to respond only for valid neighbours

Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

	Appended is a patch that modifies proxy_arp to check
the target's neighbour state before reporting any information
to requestors. The benefit is that the availability of the target
host is properly reported to the requestor, we prefer not to send
replies when target is unreachable. The changes:

- now pneigh_lookup has more priority compared to arp_fwd_proxy

- neigh_event_ns is called only once per request, not twice

- the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
the requestor should have same information no matter the packet
type. In all cases we add response delay for all requests, broadcast
or unicast, to help other authoritative hosts to reply before us.

- the RTCF_DNAT case is not delayed anymore (may be it is not used
anymore, IIRC)

Remaining questions:

- do we need to delay the pneigh_lookup case? Now it is still delayed
as before.

Comments?

Regards

--
Julian Anastasov <ja@ssi.bg>

diff -ur v2.6.2/linux/net/ipv4/arp.c linux/net/ipv4/arp.c
--- v2.6.2/linux/net/ipv4/arp.c	2004-02-05 00:23:18.000000000 +0200
+++ linux/net/ipv4/arp.c	2004-02-08 21:06:49.843123528 +0200
@@ -758,24 +758,36 @@
 			}
 			goto out;
 		} else if (IN_DEV_FORWARD(in_dev)) {
+			int fwd_proxy = 0, delay = 0;
+
 			if ((rt->rt_flags&RTCF_DNAT) ||
 			    (addr_type == RTN_UNICAST  && rt->u.dst.dev != dev &&
-			     (arp_fwd_proxy(in_dev, rt) || pneigh_lookup(&arp_tbl, &tip, dev, 0)))) {
-				n = neigh_event_ns(&arp_tbl, sha, &sip, dev);
-				if (n)
-					neigh_release(n);
-
-				if (skb->stamp.tv_sec == 0 ||
-				    skb->pkt_type == PACKET_HOST ||
-				    in_dev->arp_parms->proxy_delay == 0) {
-					arp_send(ARPOP_REPLY,ETH_P_ARP,sip,dev,tip,sha,dev->dev_addr,sha);
-				} else {
-					pneigh_enqueue(&arp_tbl, in_dev->arp_parms, skb);
-					in_dev_put(in_dev);
-					return 0;
-				}
+			     ((delay = in_dev->arp_parms->proxy_delay,
+			       pneigh_lookup(&arp_tbl, &tip, dev, 0)) ||
+			      (fwd_proxy = arp_fwd_proxy(in_dev, rt))))) {
+
+				if (skb->stamp.tv_sec) {
+					n = neigh_event_ns(&arp_tbl, sha, &sip, dev);
+					if (n)
+						neigh_release(n);
+					if ((fwd_proxy &&
+					    (n = rt->u.dst.neighbour) &&
+					    /* We need the actual state ASAP */
+					    neigh_event_send(n, NULL)) || delay) {
+						if (delay) {
+							pneigh_enqueue(&arp_tbl, in_dev->arp_parms, skb);
+							skb = NULL;
+						}
+						goto out;
+					}
+				} else if (fwd_proxy && (n = rt->u.dst.neighbour) &&
+					   !neigh_is_valid(n))
+					goto out;
+
+				arp_send(ARPOP_REPLY,ETH_P_ARP,sip,dev,tip,sha,dev->dev_addr,sha);
+				goto out;
+			} else if (!skb->stamp.tv_sec)
 				goto out;
-			}
 		}
 	}
 
@@ -819,7 +831,8 @@
 out:
 	if (in_dev)
 		in_dev_put(in_dev);
-	kfree_skb(skb);
+	if (skb)
+		kfree_skb(skb);
 	return 0;
 }
 

Re: Change proxy_arp to respond only for valid neighbours

[David S. Miller]

On Sun, 8 Feb 2004 22:44:05 +0200 (EET)
Julian Anastasov <ja@ssi.bg> wrote:

> Comments?

This is a dicey situation in that when the entry does become
valid this does not trigger a proxy arp by us, does it?

I guess the original requestor will redo the request, so I'm
just mentioning this in passing.  This is an area where admittedly
I'm not well versed.

So proxy ARP experts speak up :)

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Sun, 8 Feb 2004, David S. Miller wrote:

> This is a dicey situation in that when the entry does become
> valid this does not trigger a proxy arp by us, does it?

	Yes, currently it works in this way, when target
replies we do not propagate this answer to the proxy_queue.
But this is not fatal becuase we apply delay in the answer
which means we have time to resolve the target. If we do not
have answer in that time we simply drop the skb from proxy_queue.
The requestor can send next requests and we reply each that
request only if the target is in valid state.

> I guess the original requestor will redo the request, so I'm

	Yes, the skbs from proxy_queue are removed in time, they
do not stay there until we have answer for them.

> just mentioning this in passing.  This is an area where admittedly
> I'm not well versed.
>
> So proxy ARP experts speak up :)

	Yep, lets see what Alexey and other gurus think

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Sun, 8 Feb 2004, David S. Miller wrote:

> So proxy ARP experts speak up :)

	I found that it is better not to delay answers to unicast
probes. In case I change other things, I'll keep latest version of this 
patch here:

http://www.ssi.bg/~ja/tmp/
arp_proxy-*.diff

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[Pekka Savola]

On Sun, 8 Feb 2004, Julian Anastasov wrote:
> 	Appended is a patch that modifies proxy_arp to check
> the target's neighbour state before reporting any information
> to requestors. The benefit is that the availability of the target
> host is properly reported to the requestor, we prefer not to send
> replies when target is unreachable. 

As this seems to make the proxy_arp more intelligent, maybe you should 
take a look at:

http://www.ietf.org/internet-drafts/draft-thaler-ipv6-ndproxy-01.txt

(which, at the moment, specifies proxy arp/ND for both IPv4 and IPv6)

and see how close our implementation is wrt. to that spec, whether the 
above spec is better or worse, or whether there may be room for 
improvement otherwise as well?

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Mon, 9 Feb 2004, Pekka Savola wrote:

> As this seems to make the proxy_arp more intelligent, maybe you should
> take a look at:
>
> http://www.ietf.org/internet-drafts/draft-thaler-ipv6-ndproxy-01.txt
>
> (which, at the moment, specifies proxy arp/ND for both IPv4 and IPv6)
>
> and see how close our implementation is wrt. to that spec, whether the
> above spec is better or worse, or whether there may be room for
> improvement otherwise as well?

	Thanks, I'll check it later today.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

On Sun, 2004-02-08 at 15:44, Julian Anastasov wrote:
> 	Hello,
> 
> 	Appended is a patch that modifies proxy_arp to check
> the target's neighbour state before reporting any information
> to requestors. The benefit is that the availability of the target
> host is properly reported to the requestor, we prefer not to send
> replies when target is unreachable. 

It should probably be a sysctl example proxy_arp_validate_neigh.
I am happy with the way it is right now in the situation i use it in.

> The changes:
> 
> - now pneigh_lookup has more priority compared to arp_fwd_proxy
>
> - neigh_event_ns is called only once per request, not twice

Could the neighbor be removed while we have the packet in the proxy
queue?
It doesnt seem like theres any harm done in redoing the neigh_event_ns
twice.

> - the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
> the requestor should have same information no matter the packet
> type. In all cases we add response delay for all requests, broadcast
> or unicast, to help other authoritative hosts to reply before us.
> 

So far we havent been delaying responses to unicast because their
state is typically sane. I should say we are taking for granted
the sender's state machine is sane and would send a valid unicast
probe. Are you protecting against insane implementations?
Other than Linux what other OS actually sends unicast probes?

> - the RTCF_DNAT case is not delayed anymore (may be it is not used
> anymore, IIRC)
> 

Looking at 2.6.1 CONFIG_IP_ROUTE_NAT RTCF_NAT is still in use. Recommend
leaving it.

> Remaining questions:
> 
> - do we need to delay the pneigh_lookup case? Now it is still delayed
> as before.

Didnt follow.

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Mon, 9 Feb 2004, jamal wrote:

> It should probably be a sysctl example proxy_arp_validate_neigh.
> I am happy with the way it is right now in the situation i use it in.

	I assume you need to probe with ARP request whether for some
IP we have:

- proxy entry

or

- we can respond for this target IP due to indev/proxy_arp set to 1

	Then where is better such flag to exist, for indev or
for outdev?

> > The changes:
> >
> > - now pneigh_lookup has more priority compared to arp_fwd_proxy
> >
> > - neigh_event_ns is called only once per request, not twice
>
> Could the neighbor be removed while we have the packet in the proxy
> queue?
> It doesnt seem like theres any harm done in redoing the neigh_event_ns
> twice.

	You are right, if proxy_delay is large the requestor can
disappear from our cache but it is not fatal for us to send reply,
we do not need cache entry for this. I'm not sure what is better, is
it right to confirm the requestor without receiving recent event
from it? If will be visible for too large proxy_delay values.

> > - the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
> > the requestor should have same information no matter the packet
> > type. In all cases we add response delay for all requests, broadcast
> > or unicast, to help other authoritative hosts to reply before us.
> >
>
> So far we havent been delaying responses to unicast because their
> state is typically sane. I should say we are taking for granted
> the sender's state machine is sane and would send a valid unicast
> probe. Are you protecting against insane implementations?
> Other than Linux what other OS actually sends unicast probes?

	I already fixed that, if we have valid answer there is no good
reason to delay it if the probe was unicast.

> > - the RTCF_DNAT case is not delayed anymore (may be it is not used
> > anymore, IIRC)
> >
>
> Looking at 2.6.1 CONFIG_IP_ROUTE_NAT RTCF_NAT is still in use. Recommend
> leaving it.

	But there is no need to apply delay in this case.

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

On Mon, 2004-02-09 at 17:23, Julian Anastasov wrote:
> 	Hello,
> 
> On Mon, 9 Feb 2004, jamal wrote:
> 
> > It should probably be a sysctl example proxy_arp_validate_neigh.
> > I am happy with the way it is right now in the situation i use it in.
> 
> 	I assume you need to probe with ARP request whether for some
> IP we have:
> 
> - proxy entry
> 
> or
> 
> - we can respond for this target IP due to indev/proxy_arp set to 1
> 
> 	Then where is better such flag to exist, for indev or
> for outdev?

parse error. Are you agreeing it should be a sysctl?
what i meant was i am happy with the slight delay in some cases i get
now; example host we are parping for sends a unicast probe - we would
immediately respond. Host sends a resolved IP packet - we attempt to
route and worst case we find that we dont have the target entry in our
cache; we arp at that point.
In your case, you will amortize that cost at arp time. In the case of
unicast probes (assuming a sane arp implementation on the other side)
you will actually be adding cost since mostly that entry will be in the
cache.
In the case of broadcasts you could achieve the same effect by setting
the proxy_delay in /proc to zero.

> > Could the neighbor be removed while we have the packet in the proxy
> > queue?
> > It doesnt seem like theres any harm done in redoing the neigh_event_ns
> > twice.
> 
> 	You are right, if proxy_delay is large the requestor can
> disappear from our cache but it is not fatal for us to send reply,
> we do not need cache entry for this. I'm not sure what is better, is
> it right to confirm the requestor without receiving recent event
> from it? If will be visible for too large proxy_delay values.

I guess you could look at one view as possibly doing 2 neigh_event_ns()
and correct whereas the other is doing 1 but with the above side effect.

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Tue, 9 Feb 2004, jamal wrote:

> > 	Then where is better such flag to exist, for indev or
> > for outdev?
>
> parse error. Are you agreeing it should be a sysctl?

	May be, if we found usage for this, because I see the
handling in this way (I should put this info in doc file later):

- if the target is not an ARP neighbour then it is already validated
from ip_route_input (for example, target is some IP on PPP device
and this device exists and is UP, so there is route we are using).
In such case we do not need a validation phase (but may be we still
need to delay the answer)

- we need delay (if configured via dev/proxy_delay) not for target
validation reasons but just to lose the game with other authoritative
hosts. For this reason we do not want to delay answers to unicast
probes if we have such answer. There is one bad case here:
authoritative hosts running in promisc mode. In such case we
risk to win the race.

	As for the static proxy entries may be we do not want
to delay the answers for them, I assume they are something like
local addresses at ARP level? So, do we need to delay the
pneigh_lookup case (fwd_proxy=0)?

> what i meant was i am happy with the slight delay in some cases i get

	Ah, this delay will stay, if configured. But it is
not needed for the unicast case.

> now; example host we are parping for sends a unicast probe - we would
> immediately respond. Host sends a resolved IP packet - we attempt to
> route and worst case we find that we dont have the target entry in our
> cache; we arp at that point.

	With the new changes we will respond to unicast probe
immediately only if the target neighbour is marked valid in our
cache. For non-ARP target devices the behaviour is same - immediate
response.

	You forgot the main reason I started this change: for
neighbour state detection reasons it is bad the requestor to receive
answer for target host when this host is down. The goal is to
stop any traffic to target if it is not reachable and to use
other paths. Note that you have exactly such behaviour if target
is routed via non-ARP device, ip_route_input fails and there is no
answer. Assume such setup:

- many end hosts/routers using TIP as gw IP

- TIP is proxied from other routers runing proxy_arp

- above these routers are the uplink routers (DSL?) that have
TIP configured

	We want the end host to know the real TIP state and
to use alternative paths. So, we want to proxy the neighbour
state.

	But in what I'm not sure yet is whether there is a
usage that relies on immediate answer no matter the state.
If we do not want to ignore such usage we have to add flag
as you proposed. The question is: is it useful to provide
immediate response as before without knowing the neighbour
state, for the ARP cases.

> In your case, you will amortize that cost at arp time. In the case of
> unicast probes (assuming a sane arp implementation on the other side)
> you will actually be adding cost since mostly that entry will be in the
> cache.

	You mean the delay? I add it for other purposes, even
if target is valid in the cache.

> In the case of broadcasts you could achieve the same effect by setting
> the proxy_delay in /proc to zero.

	True, if the administrator is sure that our box is the
only responder for such targets he can set the delay to 0 to
speedup the answers.

> > 	You are right, if proxy_delay is large the requestor can
> > disappear from our cache but it is not fatal for us to send reply,
> > we do not need cache entry for this. I'm not sure what is better, is
> > it right to confirm the requestor without receiving recent event
> > from it? If will be visible for too large proxy_delay values.
>
> I guess you could look at one view as possibly doing 2 neigh_event_ns()
> and correct whereas the other is doing 1 but with the above side effect.

	Our replies are always unicast to the lladdr. Do you still
prefer 2 calls to neigh_event_ns (sorry, my english is too limited
to understand correctly the above two lines :)) ?

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

Hi Julian,

On Mon, 2004-02-09 at 18:52, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 9 Feb 2004, jamal wrote:
> 
> > > 	Then where is better such flag to exist, for indev or
> > > for outdev?
> >
> > parse error. Are you agreeing it should be a sysctl?
> 
> 	May be, if we found usage for this, because I see the
> handling in this way (I should put this info in doc file later):
> 
> - if the target is not an ARP neighbour then it is already validated
> from ip_route_input (for example, target is some IP on PPP device
> and this device exists and is UP, so there is route we are using).

Is this always guaranteed? Example "ip route get" will always create
a cache entry but not a neighbor.

> In such case we do not need a validation phase (but may be we still
> need to delay the answer)
> 
> - we need delay (if configured via dev/proxy_delay) not for target
> validation reasons but just to lose the game with other authoritative
> hosts. For this reason we do not want to delay answers to unicast
> probes if we have such answer. There is one bad case here:
> authoritative hosts running in promisc mode. In such case we
> risk to win the race.
> 

This is true, but not in my setup where i guarantee there will be
no other authoritative response.
I think authoritative answer is the main reason for the race;
the fact that you can set proxy_delay to 0 when you need to (such as in
my case) is needed flexibility.

> 	As for the static proxy entries may be we do not want
> to delay the answers for them, I assume they are something like
> local addresses at ARP level? So, do we need to delay the
> pneigh_lookup case (fwd_proxy=0)?
> 

i am indiferent but would think the current behavior is fine.

> > what i meant was i am happy with the slight delay in some cases i get
> 
> 	Ah, this delay will stay, if configured. But it is
> not needed for the unicast case.
> 

ok

> > now; example host we are parping for sends a unicast probe - we would
> > immediately respond. Host sends a resolved IP packet - we attempt to
> > route and worst case we find that we dont have the target entry in our
> > cache; we arp at that point.
> 
> 	With the new changes we will respond to unicast probe
> immediately only if the target neighbour is marked valid in our
> cache. For non-ARP target devices the behaviour is same - immediate
> response.
> 

again back to my earlier question (and talking about ARP only): 
A host would only send us a unicast probe to begin with if it is
NUD_PROBE state (iirc); which means given the exchanges the cache entry
we have would more than likely be valid still i.e if you want to
optimize this portion you will be mostly doing a useless call. Agreed?

I suppose you are trying to shortcut this by not waiting until the arp
state machine takes effect - which is fine but i claim needs to be 
configurable over current behavior.

> You forgot the main reason I started this change: for
> neighbour state detection reasons it is bad the requestor to receive
> answer for target host when this host is down. The goal is to
> stop any traffic to target if it is not reachable and to use
> other paths. 

Ok, i wasnt aware of why you started this - its a neat hack which will
improve failover times; the SCTP folks would probably like this.
 
> Note that you have exactly such behaviour if target
> is routed via non-ARP device, ip_route_input fails and there is no
> answer. Assume such setup:
> 
> - many end hosts/routers using TIP as gw IP
> 
> - TIP is proxied from other routers runing proxy_arp
> 
> - above these routers are the uplink routers (DSL?) that have
> TIP configured
> 
> 	We want the end host to know the real TIP state and
> to use alternative paths. So, we want to proxy the neighbour
> state.
> 
> 	But in what I'm not sure yet is whether there is a
> usage that relies on immediate answer no matter the state.

I think that if you want fast discovery the way you are proposing it
is the better way. Of course this comes at the expense of extra
checks (even when they are unneeded as i have claimed so far in the
case of unicast probes)

> If we do not want to ignore such usage we have to add flag
> as you proposed. The question is: is it useful to provide
> immediate response as before without knowing the neighbour
> state, for the ARP cases.

I think knowing the state before response in itself is useful when
needed. This is the most useful thing in your patch; the other parts
you are throwing in as gravy on fries and they are a little 
suspicious ;->
The main thing is backward compatibility; for years this is the
way it has worked. In my setup for example i have no need for
your shortcut.

> > In your case, you will amortize that cost at arp time. In the case of
> > unicast probes (assuming a sane arp implementation on the other side)
> > you will actually be adding cost since mostly that entry will be in the
> > cache.
> 
> 	You mean the delay? I add it for other purposes, even
> if target is valid in the cache.

Just the extra call to check state before responding adds a little to
the overhead for no good reason.
If the arp cache is invalid when you respond, the principle of
conservation of work says that work will be done later, you just defered
it to route lookup time when an IP packet is sent.

> > In the case of broadcasts you could achieve the same effect by setting
> > the proxy_delay in /proc to zero.
> 
> 	True, if the administrator is sure that our box is the
> only responder for such targets he can set the delay to 0 to
> speedup the answers.
> 

Exactly my setup. So in this case i think this feature should stay.

> > > 	You are right, if proxy_delay is large the requestor can
> > > disappear from our cache but it is not fatal for us to send reply,
> > > we do not need cache entry for this. I'm not sure what is better, is
> > > it right to confirm the requestor without receiving recent event
> > > from it? If will be visible for too large proxy_delay values.
> >
> > I guess you could look at one view as possibly doing 2 neigh_event_ns()
> > and correct whereas the other is doing 1 but with the above side effect.
> 
> 	Our replies are always unicast to the lladdr. Do you still
> prefer 2 calls to neigh_event_ns (sorry, my english is too limited
> to understand correctly the above two lines :)) ?

You used the word "fatal";-> so i was pointing that the other option is
not fatal and it happens only when you queue to the proxy q.

I think i kept you busy and i am not really an parp expert, just a 
user ;->
Now where is Alexey when you need him? ;->

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Tue, 9 Feb 2004, jamal wrote:

> Is this always guaranteed? Example "ip route get" will always create
> a cache entry but not a neighbor.

	rt_intern_hash shows that it is created and I also checked
it by using printk, the entry is freed some time after the routing
cache entry is deleted, later may be when dst is deleted and
neigh_periodic_timer removes it.

> This is true, but not in my setup where i guarantee there will be
> no other authoritative response.
> I think authoritative answer is the main reason for the race;
> the fact that you can set proxy_delay to 0 when you need to (such as in
> my case) is needed flexibility.

	So, a device flag seems as the only alternative to say
that you really want immediate answer no matter what the target
state is.

> > 	With the new changes we will respond to unicast probe
> > immediately only if the target neighbour is marked valid in our
> > cache. For non-ARP target devices the behaviour is same - immediate
> > response.
> >
>
> again back to my earlier question (and talking about ARP only):
> A host would only send us a unicast probe to begin with if it is
> NUD_PROBE state (iirc); which means given the exchanges the cache entry
> we have would more than likely be valid still i.e if you want to
> optimize this portion you will be mostly doing a useless call. Agreed?

	Yes, requestor can be in PROBE state sending unicasts
but for us the target can be already unreachable.

> I suppose you are trying to shortcut this by not waiting until the arp
> state machine takes effect - which is fine but i claim needs to be
> configurable over current behavior.

	Sometimes when delay is not 0 the immediate neigh_event_send
has chance to learn the target's state before the request is
dequeued for answer. But if delay is configured to 0 we have to
drop the first request because we do not have real answer, we
have to wait for 2nd request. The goal is not to give false answers
even for unicast requests. To avoid such one-second delay we can
walk the proxy_queue when target answers and to propagate the
answer to all queued requests but it will take too many CPU cycles,
I think. So, we have three options when delay is set to 0:

1. the first request is dropped if there is no valid entry for target

2. we lie and send false answers to unicast probes, for long time
after target becomes unreachable

3. we introduce intentional delay (the configured delay is 0),
we queue this request and later probably reply to it

I can agree with you (for case 2) only if the requestor is
not going to send unicast probes forever. But looking at the
end of arp_process if Linux is the requestor it will enter
NUD_REACHABLE state after receiving unicast reply. So, may
be this is going to live forever? It seems the periodic timer
is going to loop between
NUD_REACHABLE -> NUD_STALE -> NUD_DELAY -> NUD_PROBE (sending
unicasts) -> and then we receive false unicast reply -> loop

> > You forgot the main reason I started this change: for
> > neighbour state detection reasons it is bad the requestor to receive
> > answer for target host when this host is down. The goal is to
> > stop any traffic to target if it is not reachable and to use
> > other paths.
>
> Ok, i wasnt aware of why you started this - its a neat hack which will
> improve failover times; the SCTP folks would probably like this.

	But it will drop first requests if delay is set to 0 :)

> > 	But in what I'm not sure yet is whether there is a
> > usage that relies on immediate answer no matter the state.
>
> I think that if you want fast discovery the way you are proposing it
> is the better way. Of course this comes at the expense of extra
> checks (even when they are unneeded as i have claimed so far in the
> case of unicast probes)

	It seems they are needed if we want to provide valid answers

> > If we do not want to ignore such usage we have to add flag
> > as you proposed. The question is: is it useful to provide
> > immediate response as before without knowing the neighbour
> > state, for the ARP cases.
>
> I think knowing the state before response in itself is useful when
> needed. This is the most useful thing in your patch; the other parts
> you are throwing in as gravy on fries and they are a little
> suspicious ;->
> The main thing is backward compatibility; for years this is the
> way it has worked. In my setup for example i have no need for
> your shortcut.

	Yes, it seems it serves only failover purposes

> > > In your case, you will amortize that cost at arp time. In the case of
> > > unicast probes (assuming a sane arp implementation on the other side)
> > > you will actually be adding cost since mostly that entry will be in the
> > > cache.
> >
> > 	You mean the delay? I add it for other purposes, even
> > if target is valid in the cache.
>
> Just the extra call to check state before responding adds a little to
> the overhead for no good reason.

	The good news is that it is cached during the reachable_time :)

> If the arp cache is invalid when you respond, the principle of
> conservation of work says that work will be done later, you just defered
> it to route lookup time when an IP packet is sent.

	The main thing is that I do not want the requestor to add
routing cache entry for this dead path because such entries are going
to flood us with IP [re]transmissions which is not needed. The best way
is the requestor to avoid the failed target IP as gateway and to cache
another (probably) reachable target. No other benefits, I think.
For this, the requestor should switch from PROBE to FAILED.

> > 	True, if the administrator is sure that our box is the
> > only responder for such targets he can set the delay to 0 to
> > speedup the answers.
> >
>
> Exactly my setup. So in this case i think this feature should stay.

	So, how are we going to support it? Additional flag?
If we do not support it we are going to drop the first request
and to answer the next one. Or may be we can introduce delay?

> I think i kept you busy and i am not really an parp expert, just a
> user ;->

	Thank you for your time :) We should check this issue
from all perspectives.

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

On Tue, 2004-02-10 at 04:44, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 9 Feb 2004, jamal wrote:
> 
> > Is this always guaranteed? Example "ip route get" will always create
> > a cache entry but not a neighbor.
> 
> 	rt_intern_hash shows that it is created and I also checked
> it by using printk, the entry is freed some time after the routing
> cache entry is deleted, later may be when dst is deleted and
> neigh_periodic_timer removes it.
> 

I believe it would work even if there is no neighbor resolved yet.
i.e it depends only on a route being resolvable not necessarily
having the next hop existing already in arp cache. Try get on some host
that doesnt exist in cache yet - try on some fake address on a directly
connected subnet (make sure you pick a host that doesnt exist and
therefore will never be reached).

> > This is true, but not in my setup where i guarantee there will be
> > no other authoritative response.
> > I think authoritative answer is the main reason for the race;
> > the fact that you can set proxy_delay to 0 when you need to (such as in
> > my case) is needed flexibility.
> 
> 	So, a device flag seems as the only alternative to say
> that you really want immediate answer no matter what the target
> state is.

nod.

> >
> > again back to my earlier question (and talking about ARP only):
> > A host would only send us a unicast probe to begin with if it is
> > NUD_PROBE state (iirc); which means given the exchanges the cache entry
> > we have would more than likely be valid still i.e if you want to
> > optimize this portion you will be mostly doing a useless call. Agreed?
> 
> 	Yes, requestor can be in PROBE state sending unicasts
> but for us the target can be already unreachable.

This is true; what i am saying is for this to happen more than likely
something odd must have happened ex: the cable towards the target may
have been pulled.i.e the chances of this happening just because the
arp cache expired are low. The arp cache would exist because the
initial states would have created it.

> > I suppose you are trying to shortcut this by not waiting until the arp
> > state machine takes effect - which is fine but i claim needs to be
> > configurable over current behavior.
> 
> 	Sometimes when delay is not 0 the immediate neigh_event_send
> has chance to learn the target's state before the request is
> dequeued for answer. But if delay is configured to 0 we have to
> drop the first request because we do not have real answer, we
> have to wait for 2nd request. The goal is not to give false answers
> even for unicast requests. 

But the above assumes there are false alarms in all cases ;->

> To avoid such one-second delay we can
> walk the proxy_queue when target answers and to propagate the
> answer to all queued requests but it will take too many CPU cycles,
> I think. So, we have three options when delay is set to 0:
> 
> 1. the first request is dropped if there is no valid entry for target
> 
> 2. we lie and send false answers to unicast probes, for long time
> after target becomes unreachable

This is current behavior; i wouldnt say we lie rather we send educated
response back. 

> 
> 3. we introduce intentional delay (the configured delay is 0),
> we queue this request and later probably reply to it
> 
> I can agree with you (for case 2) only if the requestor is
> not going to send unicast probes forever. 

Thats why i asked if you are trying to catch insane implementation.
In my case everything is Linux. Ok, theres some pollution with
a CISCO upstream, but that seems to have a sane arp too.
Leave #2 with a flag to ask for for neigh_event_send() to be used
when needed.

> But looking at the
> end of arp_process if Linux is the requestor it will enter
> NUD_REACHABLE state after receiving unicast reply. So, may
> be this is going to live forever? It seems the periodic timer
> is going to loop between
> NUD_REACHABLE -> NUD_STALE -> NUD_DELAY -> NUD_PROBE (sending
> unicasts) -> and then we receive false unicast reply -> loop

Well, yeah - an insane implementation will cause a lot of grief;

[..]

> > > 	You mean the delay? I add it for other purposes, even
> > > if target is valid in the cache.
> >
> > Just the extra call to check state before responding adds a little to
> > the overhead for no good reason.
> 
> 	The good news is that it is cached during the reachable_time :)
> 
> > If the arp cache is invalid when you respond, the principle of
> > conservation of work says that work will be done later, you just defered
> > it to route lookup time when an IP packet is sent.
> 
> 	The main thing is that I do not want the requestor to add
> routing cache entry for this dead path because such entries are going
> to flood us with IP [re]transmissions which is not needed. The best way
> is the requestor to avoid the failed target IP as gateway and to cache
> another (probably) reachable target. No other benefits, I think.
> For this, the requestor should switch from PROBE to FAILED.
> 

I dont question the validity of this portion of your patch.
i.e it is an improvement in certain cases - the cost is only an extra
neigh_event_send(). But because it is not needed 100% of the time given
the educated assumption being made right now, i think the behavior
should be configurable. For me #2 breaks only if the target dissapeares
(cable gone or target removed). 

> > > 	True, if the administrator is sure that our box is the
> > > only responder for such targets he can set the delay to 0 to
> > > speedup the answers.
> > >
> >
> > Exactly my setup. So in this case i think this feature should stay.
> 
> 	So, how are we going to support it? Additional flag?
> If we do not support it we are going to drop the first request
> and to answer the next one. Or may be we can introduce delay?

We should support it. It should just be turned off by default.
It seems to me it should be per device flag with ability to turn it off
for all devices too, no?

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Tue, 10 Feb 2004, jamal wrote:

> I believe it would work even if there is no neighbor resolved yet.
> i.e it depends only on a route being resolvable not necessarily
> having the next hop existing already in arp cache. Try get on some host

	Yes, I tried it on empty system, just ip route get -> NUD_NONE.

> that doesnt exist in cache yet - try on some fake address on a directly
> connected subnet (make sure you pick a host that doesnt exist and
> therefore will never be reached).

	I did it during my initial tests and now I tested it
again, looks as expected.

> > 	Yes, requestor can be in PROBE state sending unicasts
> > but for us the target can be already unreachable.
>
> This is true; what i am saying is for this to happen more than likely
> something odd must have happened ex: the cable towards the target may
> have been pulled.i.e the chances of this happening just because the
> arp cache expired are low. The arp cache would exist because the
> initial states would have created it.

	Yes, target can exist in the cache in NUD_FAILED without
timer. But it is possible if we send probe to receive answer.
So, before calling neigh_event_send we can see that the target entry
is in one of these states:

a) NUD_NONE - just created, nobody uses it, will go to NUD_INCOMPLETE
	with timer on neigh_event_send
b) NUD_INCOMPLETE - we are probing it with broadcasts, timer is running,
	will continue with the probes
c) NUD_FAILED - no timer pending, will go to NUD_INCOMPLETE to refresh
	the state
d) NUD_VALID - stale/probed/delayed/reachable (probably start delay+probe
process). STALE looks like valid state but can be outdated - the
routing cache can keep it for minutes. We can only hope that the
user has all timeouts configured properly and this period is short.

	So, if the entry is not valid we can check if ->probes
ensures at least "2 brd probes sent" (probes >= ucast_probes+2).
This means "we have no answer for the 1st probe" and allows us
to stop queueing requests. So, we need to queue requests
only if probes < ucast_probes+2 (cases a, c and sometimes b).
Of course, if the entry is valid we will queue the brd requests.
Do you like such checks instead of adding new flag? Or the wishes
are already too many to continue with this patch?

	If you prefer you can take the 20 lines of code from
http://www.ssi.bg/~ja/tmp/arp_proxy-2.6.2-1.diff and to write
a pseudo-C example how you feel the handling should be.

	For now I'm adding this:

out_delay:
	/*
	 * Do not enqueue requests after sending the 2nd brd probe,
	 * assume the target will be down for long time
	 */
	if (delay && atomic_read(&n->probes) < n->parms->ucast_probes + 2) {
		pneigh_enqueue(&arp_tbl, in_dev->arp_parms, skb);
		skb = NULL;
	}
	goto out;

> > 2. we lie and send false answers to unicast probes, for long time
> > after target becomes unreachable
>
> This is current behavior; i wouldnt say we lie rather we send educated
> response back.

	To clear this issue, do you mean this case 2 is for entry in
STALE state? And because we are not probing it its state in our cache is
outdated. You prefer in this case to send answer, right? Because I
do not see a good reason to send answer for FAILED/INCOMPLETE/NONE.
And because STALE is part of VALID we always will send answer,
no matter brd/uni. I do not see where we need to send ARP reply
for !NUD_VALID.

> > I can agree with you (for case 2) only if the requestor is
> > not going to send unicast probes forever.
>
> Thats why i asked if you are trying to catch insane implementation.
> In my case everything is Linux. Ok, theres some pollution with
> a CISCO upstream, but that seems to have a sane arp too.
> Leave #2 with a flag to ask for for neigh_event_send() to be used
> when needed.

	I only worry if you want to send replies to unicast
probes in FAILED/INCOMPLETE/NONE state. If we have valid neighbour,
even STALE one, we can send reply no matter the uni/brd type.
brd/uni are differently treated only for the delay but we should
ignore both uni and brd requests if the entry is !VALID.

	I think, it is not enough the flag to control whether
neigh_event_send is used, if the flag is 0 you want also to
send replies in !VALID state (as before) because nobody will
send us IP traffic without receiving answer from us - only
IP traffic can originate resolution in such case. For now, I
see that the flag can be only in this way (on output interface?):

- 0: (default, as before): send answers in !VALID, do not use
neigh_event_send. The problem with the old behaviour is that
we always send answer, even in !VALID.

- 1: (new): do not send answers in !VALID, use neigh_event_send

	do you mean the same?

> I dont question the validity of this portion of your patch.
> i.e it is an improvement in certain cases - the cost is only an extra
> neigh_event_send(). But because it is not needed 100% of the time given

	And I do not see any cost in using neigh_event_send,
you are always going to fire the resolution process, now with ARP
probes or later with IP traffic in the default mode. The ARP
traffic to/from target will be equal in both cases.

> the educated assumption being made right now, i think the behavior
> should be configurable. For me #2 breaks only if the target dissapeares
> (cable gone or target removed).

	I hope you can clarify your idea with pseudo-C example :)

> > > Exactly my setup. So in this case i think this feature should stay.
> >
> > 	So, how are we going to support it? Additional flag?
> > If we do not support it we are going to drop the first request
> > and to answer the next one. Or may be we can introduce delay?
>
> We should support it. It should just be turned off by default.
> It seems to me it should be per device flag with ability to turn it off
> for all devices too, no?

	Yes, the flag can be (all/flag || iface/flag), no problem with
that :)

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

Hi,

On Tue, 2004-02-10 at 20:52, Julian Anastasov wrote: 
> 	Hello,
> 
> On Tue, 10 Feb 2004, jamal wrote:
> 
> > I believe it would work even if there is no neighbor resolved yet.
> > i.e it depends only on a route being resolvable not necessarily
> > having the next hop existing already in arp cache. Try get on some host
> 
> 	Yes, I tried it on empty system, just ip route get -> NUD_NONE.

It doesnt seem to show up at all in the neigh cache display although
you are correct that it does go to NUD_NONE given the transition
when you attempt a ping right after. 

is NUD_NONE never displayed by ip n ls?

> > that doesnt exist in cache yet - try on some fake address on a directly
> > connected subnet (make sure you pick a host that doesnt exist and
> > therefore will never be reached).
> 
> 	I did it during my initial tests and now I tested it
> again, looks as expected.

I am not %100 sure if such an entry should be route cached if the nh
is not resolvable. What are your thoughts on this?

> > > 	Yes, requestor can be in PROBE state sending unicasts
> > > but for us the target can be already unreachable.
> >
> > This is true; what i am saying is for this to happen more than likely
> > something odd must have happened ex: the cable towards the target may
> > have been pulled.i.e the chances of this happening just because the
> > arp cache expired are low. The arp cache would exist because the
> > initial states would have created it.
> 
> 	Yes, target can exist in the cache in NUD_FAILED without
> timer. But it is possible if we send probe to receive answer.

NUD_FAILED is dead meat waiting to be garbage collected.

> So, before calling neigh_event_send we can see that the target entry
> is in one of these states:
> 
> a) NUD_NONE - just created, nobody uses it, will go to NUD_INCOMPLETE
> 	with timer on neigh_event_send
> b) NUD_INCOMPLETE - we are probing it with broadcasts, timer is running,
> 	will continue with the probes
> c) NUD_FAILED - no timer pending, will go to NUD_INCOMPLETE to refresh
> 	the state
> d) NUD_VALID - stale/probed/delayed/reachable (probably start delay+probe
> process). STALE looks like valid state but can be outdated - the
> routing cache can keep it for minutes. We can only hope that the
> user has all timeouts configured properly and this period is short.
> 
> 	So, if the entry is not valid we can check if ->probes
> ensures at least "2 brd probes sent" (probes >= ucast_probes+2).
> This means "we have no answer for the 1st probe" and allows us
> to stop queueing requests. So, we need to queue requests
> only if probes < ucast_probes+2 (cases a, c and sometimes b).
> Of course, if the entry is valid we will queue the brd requests.
> Do you like such checks instead of adding new flag? Or the wishes
> are already too many to continue with this patch?

I think if any cache entry is NUD_VALID you should respond immediately.
anything else you should enqueue request and initiate a discovery to
target.

> 	If you prefer you can take the 20 lines of code from
> http://www.ssi.bg/~ja/tmp/arp_proxy-2.6.2-1.diff and to write
> a pseudo-C example how you feel the handling should be.
> 

Ok, I looked at this and i am wondering if all logic you have is needed.
I apologize in advance because i feel i missed something you said
earlier, but what is wrong with just saying along the following lines:

 if (skb->stamp.tv_sec == 0 ||
    skb->pkt_type == PACKET_HOST ||
    in_dev->arp_parms->proxy_delay == 0) {
         if ((in_dev->arp_parms->proxy_validate) &&
              n && !neigh_is_valid(n)) {
                              /* probably issue arp to tip here */
                              goto q_it;                           
	                                                             
	}                                     
	arp_send(ARPOP_REPLY,ETH_P_ARP,sip,dev,tip,sha,dev->dev_addr,sha);
 } else {
q_it:
         if (fwd_proxy)
              neigh_event_send(n, NULL);
         pneigh_enqueue(&arp_tbl, in_dev->arp_parms, skb);
         in_dev_put(in_dev);
         return 0;
 }
 goto out;

--	


Let me cut your email in two here because it is getting too long


cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On 11 Feb 2004, jamal wrote:

> > 	Yes, I tried it on empty system, just ip route get -> NUD_NONE.
>
> It doesnt seem to show up at all in the neigh cache display although
> you are correct that it does go to NUD_NONE given the transition
> when you attempt a ping right after.
>
> is NUD_NONE never displayed by ip n ls?

	I now see that the trick is ip n l nud none

> > 	I did it during my initial tests and now I tested it
> > again, looks as expected.
>
> I am not %100 sure if such an entry should be route cached if the nh
> is not resolvable. What are your thoughts on this?

	It looks bad to call route_slow for every one request,
I have to think more about it.

> > 	Yes, target can exist in the cache in NUD_FAILED without
> > timer. But it is possible if we send probe to receive answer.
>
> NUD_FAILED is dead meat waiting to be garbage collected.

	But can be outdated.

> I think if any cache entry is NUD_VALID you should respond immediately.

	No, we should delay the broadcasts if such delay is
configured. For unicasts we respond immediately.

> anything else you should enqueue request and initiate a discovery to
> target.

	By checking ->probes we queue !VALID entries only
when switching from FAILED to INCOMPLETE. For example:

- we have 3 brd probes configured to send

- request comes

- we send 1st brd probe for target and enqueue this incoming request
and all requests coming before we send the 2nd probe

- we send 2nd probe (still no answer for the 1st)

- at this point we know that target is really dead and we do not
need to queue more incoming requests (even brd ones) until target
responds (the queue has limit)

- this cycle repeats on each 3 seconds which means that for 1
second we queue requests and for 2 seconds we just drop

> Ok, I looked at this and i am wondering if all logic you have is needed.
> I apologize in advance because i feel i missed something you said
> earlier, but what is wrong with just saying along the following lines:
>
>  if (skb->stamp.tv_sec == 0 ||
>     skb->pkt_type == PACKET_HOST ||
>     in_dev->arp_parms->proxy_delay == 0) {
>          if ((in_dev->arp_parms->proxy_validate) &&
>               n && !neigh_is_valid(n)) {
>                               /* probably issue arp to tip here */
>                               goto q_it;
>
> 	}
> 	arp_send(ARPOP_REPLY,ETH_P_ARP,sip,dev,tip,sha,dev->dev_addr,sha);
>  } else {
> q_it:
>          if (fwd_proxy)
>               neigh_event_send(n, NULL);
>          pneigh_enqueue(&arp_tbl, in_dev->arp_parms, skb);
>          in_dev_put(in_dev);
>          return 0;
>  }
>  goto out;

	There is possible loop in this example (as DaveM warned)
when ((tv_sec=0 or delay=0) and !valid) but it can be fixed (then
you will have code in similar size :)). The problem is that I
wanted to avoid creating new flag if there is no need to support
the old behavior in the real life. Also, we have choice to apply
delay according to the proxy type (DNAT/fwd/pneigh) and the
probes sent.

	Also, you do not call neigh_event_send when
request with tv_sec!=0 comes. This is bad in cases where the
requestor sends only ARP requests without IP traffic, you
risk target to switch to STALE state for long time and we will
start to provide outdated information. We have to treat the
ARP requests as IP traffic and to update the target's state.

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

On Wed, 2004-02-11 at 09:32, Julian Anastasov wrote:

> > > 	I did it during my initial tests and now I tested it
> > > again, looks as expected.
> >
> > I am not %100 sure if such an entry should be route cached if the nh
> > is not resolvable. What are your thoughts on this?
> 
> 	It looks bad to call route_slow for every one request,
> I have to think more about it.

If you make another get it goes via fast path.
The disappointing part is this feature has a lot of potential
if it works "correctly" (imagine being able to populate route cache
before packet arrives and not needing for packet to go via slow path).

>> I think if any cache entry is NUD_VALID you should respond
>>immediately.

>        No, we should delay the broadcasts if such delay is
> configured. For unicasts we respond immediately.

Ok, extend that to say "if we have to respond immediately we may check
if the target is valid"

> anything else you should enqueue request and initiate a discovery to
> target.
> 	By checking ->probes we queue !VALID entries only
> when switching from FAILED to INCOMPLETE. For example:
> 
> - we have 3 brd probes configured to send
> 
> - request comes
> 
> - we send 1st brd probe for target and enqueue this incoming request
> and all requests coming before we send the 2nd probe
> 
> - we send 2nd probe (still no answer for the 1st)
> 
> - at this point we know that target is really dead and we do not
> need to queue more incoming requests (even brd ones) until target
> responds (the queue has limit)
> 
> - this cycle repeats on each 3 seconds which means that for 1
> second we queue requests and for 2 seconds we just drop

I am indifferent about the above (and it may be complicating things). 

> 	There is possible loop in this example (as DaveM warned)
> when ((tv_sec=0 or delay=0) and !valid) but it can be fixed 

Correct
Note the point i was trying to make in that snippet is: the
valuable piece is in checking that for cases where we respond 
immediately we at least validate the state of the target.
To me thats all you need to do ;->

> (then
> you will have code in similar size :)). 
>
> The problem is that I
> wanted to avoid creating new flag if there is no need to support
> the old behavior in the real life. Also, we have choice to apply
> delay according to the proxy type (DNAT/fwd/pneigh) and the
> probes sent.
> 

Ok, remove the flag. That one piece is safe. I didnt understand
why you have to speacial case for "NAT/fwd/pneigh" 

> 	Also, you do not call neigh_event_send when
> request with tv_sec!=0 comes. 

I think we have been this whole issue severely (i.e you understand my
view and i understand yours) - lets get a closure. Heres where i stand:
- checking validity in fast response is fair game and doesnt need a flag
- loops must be avoided

Some of your features are fine except they may be a little extreme.
Note, this code has been to work well for years, the minimalist
approach to features would be the best approach.

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Thu, 11 Feb 2004, jamal wrote:

> > 	It looks bad to call route_slow for every one request,
> > I have to think more about it.
>
> If you make another get it goes via fast path.
> The disappointing part is this feature has a lot of potential
> if it works "correctly" (imagine being able to populate route cache
> before packet arrives and not needing for packet to go via slow path).

	I'm not sure I understand your idea, may be you will try
it one day :)

> > - this cycle repeats on each 3 seconds which means that for 1
> > second we queue requests and for 2 seconds we just drop
>
> I am indifferent about the above (and it may be complicating things).

	I'll have to test it first because it can reduce the
proxy_queue usage up to 3 times which can be desired in some
setups, we have linear search there.

> Ok, remove the flag. That one piece is safe. I didnt understand
> why you have to speacial case for "NAT/fwd/pneigh"

	DNAT is like local address, no need to delay at all.

> I think we have been this whole issue severely (i.e you understand my
> view and i understand yours) - lets get a closure. Heres where i stand:
> - checking validity in fast response is fair game and doesnt need a flag
> - loops must be avoided
>
> Some of your features are fine except they may be a little extreme.
> Note, this code has been to work well for years, the minimalist
> approach to features would be the best approach.

	I hope it will continue to work but in a fair way :)

	ok, we are the final of this, I have prepared a change log:

http://www.ssi.bg/~ja/tmp/CHANGES

	At this point I do not know how to optimize this patch and
all changes look valid to me. arp_proxy-2.6.2-1.diff is still the
latest version but I have to check the proxy_queue optimization.
To make the code more readable may we can move the (IN_DEV_FORWARD(in_dev))
body at the end of arp_process. And I'll do more tests in the following
days.

	I know you prefer to live with the old behaviour. I'm not
sure if we reorder the old code it will look more readable while
supporting the main new features. And I'm not going to push this if
we do not have consensus on the implementation. Let me know if
you have a better version of this change.

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

Hi Julian,

On Thu, 2004-02-12 at 04:46, Julian Anastasov wrote:

> > If you make another get it goes via fast path.
> > The disappointing part is this feature has a lot of potential
> > if it works "correctly" (imagine being able to populate route cache
> > before packet arrives and not needing for packet to go via slow path).
> 
> 	I'm not sure I understand your idea, may be you will try
> it one day :)

We can take this offline. 

> > I am indifferent about the above (and it may be complicating things).
> 
> 	I'll have to test it first because it can reduce the
> proxy_queue usage up to 3 times which can be desired in some
> setups, we have linear search there.

That would be great. If you can show a claer benefit, should be fine.


> > Some of your features are fine except they may be a little extreme.
> > Note, this code has been to work well for years, the minimalist
> > approach to features would be the best approach.
> 
> 	I hope it will continue to work but in a fair way :)

Thats what i am hoping for. Sometimes i just dont touch code
unnecessarily because it may have strange side effects; look at that
tunnel problem posted yesterday for example - some change an inequality
sign in the net sched code and ipip tunnels get affected.

> 	ok, we are the final of this, I have prepared a change log:
> 
> http://www.ssi.bg/~ja/tmp/CHANGES
> 
> 	At this point I do not know how to optimize this patch and
> all changes look valid to me. arp_proxy-2.6.2-1.diff is still the
> latest version but I have to check the proxy_queue optimization.
> To make the code more readable may we can move the (IN_DEV_FORWARD(in_dev))
> body at the end of arp_process. And I'll do more tests in the following
> days.
> 
> 	I know you prefer to live with the old behaviour. I'm not
> sure if we reorder the old code it will look more readable while
> supporting the main new features. And I'm not going to push this if
> we do not have consensus on the implementation. Let me know if
> you have a better version of this change.
> 

Post the patch and i will test it in my environment and make sure it
doesnt break anything here. My interest are two cases: default setup and
proxy_delay being 0. 

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[kuznet]

Hello!

Joining the discussion too late, but let me to reformulate original
Jamal's question in another way: are you aware that you relay _broadcast_
ARP requests doing the validation?

I really like the idea, but this aspect bothers me a lot. Is it not storm
unsafe? Look, earlier broadcasts on outgoing interface started only when
a real unicast IP traffic was pending, so we were sure sender selected some
single proxy router, now broadcasts will be sent by _all_ the routers. See?

Alexey

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Thu, 12 Feb 2004 kuznet@ms2.inr.ac.ru wrote:

> Joining the discussion too late, but let me to reformulate original
> Jamal's question in another way: are you aware that you relay _broadcast_
> ARP requests doing the validation?

	Good question. It seems I misunderstood the goal of the
delaying mechanism.

> I really like the idea, but this aspect bothers me a lot. Is it not storm
> unsafe? Look, earlier broadcasts on outgoing interface started only when
> a real unicast IP traffic was pending, so we were sure sender selected some
> single proxy router, now broadcasts will be sent by _all_ the routers. See?

	Yep, 3 broadcasts per router for one requests which is 3 times
more than the current code sends in response to fake requests.

	I thought the delay is to lose the race with hosts with
this IP but now I see it is implemented as random delay. But we never
can guarantee that only one proxy router will serve the target IP - I see
the nice effect of using all proxy routers in parallel from
different senders. Any other hidden features? :)

	As for the proposed changes, any recommendations?
May be the flag for the incoming interface as Jamal requested
is already required if we still like to see these new features
in the kernel and running in a safe environment? Or it is
better to stay only with the current behaviour?

> Alexey

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[jamal]

On Tue, 2004-02-10 at 20:52, Julian Anastasov wrote:

> > > 2. we lie and send false answers to unicast probes, for long time
> > > after target becomes unreachable
> >
> > This is current behavior; i wouldnt say we lie rather we send educated
> > response back.
> 
> 	To clear this issue, do you mean this case 2 is for entry in
> STALE state? And because we are not probing it its state in our cache is
> outdated. You prefer in this case to send answer, right? Because I
> do not see a good reason to send answer for FAILED/INCOMPLETE/NONE.
> And because STALE is part of VALID we always will send answer,
> no matter brd/uni. I do not see where we need to send ARP reply
> for !NUD_VALID.

I agree with you. We may be saying the same thing. What i mean is [**]:
further below.

> > > I can agree with you (for case 2) only if the requestor is
> > > not going to send unicast probes forever.
> >
> > Thats why i asked if you are trying to catch insane implementation.
> > In my case everything is Linux. Ok, theres some pollution with
> > a CISCO upstream, but that seems to have a sane arp too.
> > Leave #2 with a flag to ask for for neigh_event_send() to be used
> > when needed.
> 
> 	I only worry if you want to send replies to unicast
> probes in FAILED/INCOMPLETE/NONE state. If we have valid neighbour,
> even STALE one, we can send reply no matter the uni/brd type.
> brd/uni are differently treated only for the delay but we should
> ignore both uni and brd requests if the entry is !VALID.

Thats what i was also trying to say with a small caveat
in [**].

> 	I think, it is not enough the flag to control whether
> neigh_event_send is used, if the flag is 0 you want also to
> send replies in !VALID state (as before) because nobody will
> send us IP traffic without receiving answer from us - only
> IP traffic can originate resolution in such case. For now, I
> see that the flag can be only in this way (on output interface?):

Look at that sample code i sent earlier - thats what ive been meaning to 
say all along ;-> Gives meaning to code speaks louder ;->
 
> - 0: (default, as before): send answers in !VALID, do not use
> neigh_event_send. The problem with the old behaviour is that
> we always send answer, even in !VALID.
> 

yes.

> - 1: (new): do not send answers in !VALID, use neigh_event_send
> 
> 	do you mean the same?
> 

I think so.

> > I dont question the validity of this portion of your patch.
> > i.e it is an improvement in certain cases - the cost is only an extra
> > neigh_event_send(). But because it is not needed 100% of the time given
> 
> 	And I do not see any cost in using neigh_event_send,
> you are always going to fire the resolution process, now with ARP
> probes or later with IP traffic in the default mode. The ARP
> traffic to/from target will be equal in both cases.

Ok, so you agree with what i say in [**] but you are just trying to
ensure it happens all the time i think. 

> > the educated assumption being made right now, i think the behavior
> > should be configurable. For me #2 breaks only if the target dissapeares
> > (cable gone or target removed).
> 
> 	I hope you can clarify your idea with pseudo-C example :)

The previous sample i sent + [**] below.
Note that in the sample code i sent case of proxy_delay being zero even 
bcasts are treated the same way.

[..]

[**] If a linux machine (which is sane ARP implementation) sent a unicast 
probe to us, theres high likelihood that the target is in VALID state. 
This is what i refered to as an educated guess.

cheers,
jamal

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On 11 Feb 2004, jamal wrote:

> I agree with you. We may be saying the same thing. What i mean is [**]:
> further below.

	Yes, we better to talks with source code :)

> [**] If a linux machine (which is sane ARP implementation) sent a unicast
> probe to us, theres high likelihood that the target is in VALID state.
> This is what i refered to as an educated guess.

	Ah, this is not true :) Sane requestor as Linux will switch
forever between STALE (delay+probe) and REACHABLE if target is STALE
and we answer to these unicast requests without updating the
target's state, for example, if there is only ARP traffic. In
normal usage it can not occur but if someone is sending
unicast requests without IP traffic we will hit it. IIRC, old
arping versions do exactly this, 1 brd + many unicasts.

> cheers,
> jamal

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[David S. Miller]

On Sun, 8 Feb 2004 22:44:05 +0200 (EET)
Julian Anastasov <ja@ssi.bg> wrote:

> - the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
> the requestor should have same information no matter the packet
> type. In all cases we add response delay for all requests, broadcast
> or unicast, to help other authoritative hosts to reply before us.

Do we really want to reply to all the garbage tcpdump causes us
to capture?

That is what the pkt_type is dealing with.  If we're in promiscuous
mode, we'll hear ARP requests meant not for any of our devices, we
should not proxy for them right?

RTCF_*NAT is dead wood, the existing route nating stuff is totally broken
an unusable in 2.6.x, the eventual plan was to code up XFRM engine version
of that feature but this is of course not done.  Since nobody is complaining
about lack of routing NAT in 2.6.x, I think we should just kill off all references
and if someone gets inspired they can code up the XFRM engine version.

Re: Change proxy_arp to respond only for valid neighbours

[James Morris]

On Mon, 9 Feb 2004, David S. Miller wrote:

> RTCF_*NAT is dead wood, the existing route nating stuff is totally broken
> an unusable in 2.6.x, the eventual plan was to code up XFRM engine version
> of that feature but this is of course not done.  Since nobody is complaining
> about lack of routing NAT in 2.6.x, I think we should just kill off all references
> and if someone gets inspired they can code up the XFRM engine version.

Sounds like a plan.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: Change proxy_arp to respond only for valid neighbours

[Ben Greear]

David S. Miller wrote:
> On Sun, 8 Feb 2004 22:44:05 +0200 (EET)
> Julian Anastasov <ja@ssi.bg> wrote:
> 
> 
>>- the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
>>the requestor should have same information no matter the packet
>>type. In all cases we add response delay for all requests, broadcast
>>or unicast, to help other authoritative hosts to reply before us.
> 
> 
> Do we really want to reply to all the garbage tcpdump causes us
> to capture?
> 
> That is what the pkt_type is dealing with.  If we're in promiscuous
> mode, we'll hear ARP requests meant not for any of our devices, we
> should not proxy for them right?

Arp requests are always broadcasts, so promisc changes nothing,
right?


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Change proxy_arp to respond only for valid neighbours

[David S. Miller]

On Mon, 09 Feb 2004 13:39:51 -0800
Ben Greear <greearb@candelatech.com> wrote:

> Arp requests are always broadcasts, so promisc changes nothing,
> right?

Unicast probes are possible.

If it is broadcast/multicast then pkt_type will be PACKET_{BROAD,MULTI}CAST.

The existing ipv4 proxy-arp code is saying (as I read it) to not delay the
response if this is a unicast probe.  This is one of the behaviors that
Julian is modifying slightly.

For everyone's edification, when looking at changes Julian is suggesting, realize
that his interests lie in IPVS like applications, so consider the corner case
situations that such setups might be interested in and you'll see clearly the
impetus for all of his ARP change proposals :-)

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Mon, 9 Feb 2004, David S. Miller wrote:

> The existing ipv4 proxy-arp code is saying (as I read it) to not delay the
> response if this is a unicast probe.  This is one of the behaviors that
> Julian is modifying slightly.

	It will work as before if we have valid target

> For everyone's edification, when looking at changes Julian is suggesting, realize
> that his interests lie in IPVS like applications, so consider the corner case
> situations that such setups might be interested in and you'll see clearly the
> impetus for all of his ARP change proposals :-)

	You are almost right, yes, partially for arp_announce,
but IPVS like apps need more things to solve this problem. As for
this proxy_arp work it is entirely unrelated :) Hey, I'm not using only 
IPVS, it does not take my time from long long time :) Long time ago
I played with proxy_arp and was wondering why it behaves in this way.
I'm ready to leave this idea if the changes become too complex.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[David S. Miller]

On Tue, 10 Feb 2004 00:42:47 +0200 (EET)
Julian Anastasov <ja@ssi.bg> wrote:

> I'm ready to leave this idea if the changes become too complex.

I think it's not so much your changes as this huge ugly ARP receive
processing function.  It is a mockery of functional programming :-)

I even tried to split this damn thing up today into smaller chunks but
there is so much shared state (like 6 or 7 live variables that all of
the logic ends up referencing) that such efforts proved futile.

It's hard and time consuming to verify any patch to this code for
that reason.

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Mon, 9 Feb 2004, David S. Miller wrote:

> > - the 'skb->pkt_type == PACKET_HOST' check has no semantic anymore,
> > the requestor should have same information no matter the packet
> > type. In all cases we add response delay for all requests, broadcast
> > or unicast, to help other authoritative hosts to reply before us.
>
> Do we really want to reply to all the garbage tcpdump causes us
> to capture?
>
> That is what the pkt_type is dealing with.  If we're in promiscuous
> mode, we'll hear ARP requests meant not for any of our devices, we
> should not proxy for them right?

	But this is true for local TIPs too. Do we need early check for
{HOST|BROADCAST} or you prefer it only for the proxy_arp case?

> RTCF_*NAT is dead wood, the existing route nating stuff is totally broken
> an unusable in 2.6.x, the eventual plan was to code up XFRM engine version
> of that feature but this is of course not done.  Since nobody is complaining
> about lack of routing NAT in 2.6.x, I think we should just kill off all references
> and if someone gets inspired they can code up the XFRM engine version.

	I complained once but everything is doable with netfilter,
with more rules, of course.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Change proxy_arp to respond only for valid neighbours

[David S. Miller]

On Sun, 8 Feb 2004 22:44:05 +0200 (EET)
Julian Anastasov <ja@ssi.bg> wrote:

> +				if (skb->stamp.tv_sec) {

Julian, do you understand what this check is for?

If it is zero, this means we are being reinvoked via the delayed proxy queue.
It means that some previously delayed response has been kicked so that we
should respond to it right now, with no delay, no matter what.

Otherwise you could loop delaying the thing forever.  If there is a case
where some pneigh ARP request should be evaluated multiple times, such
a change belongs in the pneigh_*() code not here in ipv4 arp processing.

The exact path is:

1) All input packets get skb->stamp.tv_sec assigned to in net/core/dev.c at
   the waaaay beginning of input packet processing...

2) Therefore the first time skb goes through arp_rcv() skb->tv_sec will be
   non-zero.

3) If it is pneigh_enqueue()'d, then skb->stamp.tv_sec will be set to zero
   therefore upon reprocessing by arp_rcv() it will be seen as zero and this
   is how such a state is detected.

Yes, I hate all of these overloaded meanings of skb->stamp.tv_* as much as
anyone else.

This is my contribution wrt. this suggested patch today, it's your turn to
make some commentary Julian :-)

Re: Change proxy_arp to respond only for valid neighbours

[Julian Anastasov]

	Hello,

On Mon, 9 Feb 2004, David S. Miller wrote:

> > +				if (skb->stamp.tv_sec) {
>
> Julian, do you understand what this check is for?
>
> If it is zero, this means we are being reinvoked via the delayed proxy queue.
> It means that some previously delayed response has been kicked so that we
> should respond to it right now, with no delay, no matter what.

	Of course, there is no delay for !tv_sec, just answer or
do not answer, http://www.ssi.bg/~ja/tmp/arp_proxy-2.6.2-1.diff:

---
else
if (fwd_proxy && (n = rt->u.dst.neighbour) &&
	neigh_is_valid(n))
	goto out;
---

> Otherwise you could loop delaying the thing forever.  If there is a case
> where some pneigh ARP request should be evaluated multiple times, such
> a change belongs in the pneigh_*() code not here in ipv4 arp processing.
>
> The exact path is:
>
> 1) All input packets get skb->stamp.tv_sec assigned to in net/core/dev.c at
>    the waaaay beginning of input packet processing...
>
> 2) Therefore the first time skb goes through arp_rcv() skb->tv_sec will be
>    non-zero.
>
> 3) If it is pneigh_enqueue()'d, then skb->stamp.tv_sec will be set to zero
>    therefore upon reprocessing by arp_rcv() it will be seen as zero and this
>    is how such a state is detected.

	Absolutely correct, and it is done as you said, except if I
have some bug, of course :)

> Yes, I hate all of these overloaded meanings of skb->stamp.tv_* as much as
> anyone else.
>
> This is my contribution wrt. this suggested patch today, it's your turn to
> make some commentary Julian :-)

	You have to see the flood already :)

Regards

--
Julian Anastasov <ja@ssi.bg>