probe netlink app in NUD_PROBE

probe netlink app in NUD_PROBE

[Timo Teras]

When a stale or delayed neigh entry is being re-validated the entry
goes to NUD_PROBE state. At the moment only unicast probes are sent.
This is basically because neigh_max_probes() limits the probe amount so.

Now, opennhrp intentionally configures UCAST_PROBES and MCAST_PROBES to
zero and APP_PROBES to something meaningful. The idea is that opennhrp
replaces arp completely with NHRP implemented in userland.

Due to this it seems there is a very small time window, when the
NUD_PROBE times out and the neighbour entry gets invalidated, and
packets get lost.

To remedy this, I would like to have these NUD_PROBE validations sent
via netlink too.

First choice is to change to just use both unicast and application
probes:

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index b9e9e0d..36d3f8c 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -836,10 +836,10 @@ out:
 static __inline__ int neigh_max_probes(struct neighbour *n)
 {
 	struct neigh_parms *p = n->parms;
-	return (n->nud_state & NUD_PROBE) ?
-		NEIGH_VAR(p, UCAST_PROBES) :
-		NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES) +
-		NEIGH_VAR(p, MCAST_PROBES);
+	int max_probes = NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES);
+	if (!(n->nud_state & NUD_PROBE))
+		max_probes += NEIGH_VAR(p, MCAST_PROBES);
+	return max_probes;
 }
 
 static void neigh_invalidate(struct neighbour *neigh)

On default configuration there is no behaviour change, as APP_PROBES
defaults zero. I'm not sure if other ARPD programs than opennhrp are
currently commonly used.

If that feels risky, alternative would be:

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index b9e9e0d..8bb320b 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -836,9 +836,11 @@ out:
 static __inline__ int neigh_max_probes(struct neighbour *n)
 {
 	struct neigh_parms *p = n->parms;
-	return (n->nud_state & NUD_PROBE) ?
-		NEIGH_VAR(p, UCAST_PROBES) :
-		NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES) +
+
+	if (n->nud_state & NUD_PROBE)
+		return NEIGH_VAR(p, UCAST_PROBES) ? : NEIGH_VAR(p, APP_PROBES);
+
+	return NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES) + 
 		NEIGH_VAR(p, MCAST_PROBES);
 }
 
In which the netlink would be used only if unicast probes are turned
off.

Any preference which to send formatted formally?

- Timo

Re: probe netlink app in NUD_PROBE

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Sat, 22 Feb 2014 10:44:19 +0200

> When a stale or delayed neigh entry is being re-validated the entry
> goes to NUD_PROBE state. At the moment only unicast probes are sent.
> This is basically because neigh_max_probes() limits the probe amount so.
> 
> Now, opennhrp intentionally configures UCAST_PROBES and MCAST_PROBES to
> zero and APP_PROBES to something meaningful. The idea is that opennhrp
> replaces arp completely with NHRP implemented in userland.
> 
> Due to this it seems there is a very small time window, when the
> NUD_PROBE times out and the neighbour entry gets invalidated, and
> packets get lost.
> 
> To remedy this, I would like to have these NUD_PROBE validations sent
> via netlink too.
> 
> First choice is to change to just use both unicast and application
> probes:

So basically, the generic and protocol-specific neighbour code work
together to implement a cascading priority based probing scheme using
a per-neigh counter and three limits.

It seems that neigh->probes is zero when we enter the probing state.

On each solicit we increment neigh->probes.

Then we have this very funny logic in the protocol specific
implementations of solicit:

	probes = atomic_read(&neigh->probes);

	probes -= UCAST_PROBES;
	if (probes < 0) {
		send unicast probe;
	} else {
		probes -= APP_PROBES;
		if (probes < 0) {
			trigger application based probe via netlink
		} else {
			send multicast probe;
		}
	}

As an example, for ipv4 arp, UCAST_PROBES defaults to 3 and APP_PROBES
defaults (as you say) to zero.

If neigh_max_probes() evaluates to UCAST_PROBES, we'll do 3 unicast
probes then fail the neigh, for example.

I took a look at iproute2's arpd, and I suggest you take a gander
over there as well.

If given the '-a N' option it will set app_probes to N and send it's
own ARP requests out in certain situations.

It might depend upon the current behavior of neigh_max_probes().

Re: probe netlink app in NUD_PROBE

[Timo Teras]

On Tue, 25 Feb 2014 18:18:44 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Timo Teras <timo.teras@iki.fi>
> Date: Sat, 22 Feb 2014 10:44:19 +0200
> 
> > When a stale or delayed neigh entry is being re-validated the entry
> > goes to NUD_PROBE state. At the moment only unicast probes are sent.
> > This is basically because neigh_max_probes() limits the probe
> > amount so.
> > 
> > Now, opennhrp intentionally configures UCAST_PROBES and
> > MCAST_PROBES to zero and APP_PROBES to something meaningful. The
> > idea is that opennhrp replaces arp completely with NHRP implemented
> > in userland.
> > 
> > Due to this it seems there is a very small time window, when the
> > NUD_PROBE times out and the neighbour entry gets invalidated, and
> > packets get lost.
> > 
> > To remedy this, I would like to have these NUD_PROBE validations
> > sent via netlink too.
> > 
> > First choice is to change to just use both unicast and application
> > probes:
> 
> So basically, the generic and protocol-specific neighbour code work
> together to implement a cascading priority based probing scheme using
> a per-neigh counter and three limits.
> 
> It seems that neigh->probes is zero when we enter the probing state.
> 
> On each solicit we increment neigh->probes.
> 
> Then we have this very funny logic in the protocol specific
> implementations of solicit:
> 
> 	probes = atomic_read(&neigh->probes);
> 
> 	probes -= UCAST_PROBES;
> 	if (probes < 0) {
> 		send unicast probe;
> 	} else {
> 		probes -= APP_PROBES;
> 		if (probes < 0) {
> 			trigger application based probe via netlink
> 		} else {
> 			send multicast probe;
> 		}
> 	}
> 
> As an example, for ipv4 arp, UCAST_PROBES defaults to 3 and APP_PROBES
> defaults (as you say) to zero.

Right, the idea is that on NUD_INVALID it does sequence of:
	UCAST_PROBES protocol specific unicasts
	APP_PROBE    netlink requeusts
	MCAST_PROBES protocol specific multicasts

And in NUD_PROBE currently only the unicast probes.

> If neigh_max_probes() evaluates to UCAST_PROBES, we'll do 3 unicast
> probes then fail the neigh, for example.
> 
> I took a look at iproute2's arpd, and I suggest you take a gander
> over there as well.
> 
> If given the '-a N' option it will set app_probes to N and send it's
> own ARP requests out in certain situations.
> 
> It might depend upon the current behavior of neigh_max_probes().

In fact, it seems that the arpd is expecting to get app probe in
NUD_PROBE. There's code like:
        if (ndm->ndm_state&NUD_PROBE) {
                 /* If we get this, kernel still has some valid
                  * address, but unicast probing failed and host
                  * is either dead or changed its mac address.
                  * Kernel is going to initiate broadcast resolution.
                  * OK, we invalidate our information as well.
                  */
                  if (dbdat.data && !IS_NEG(dbdat.data))
                         stats.app_neg++;

                  dbase->del(dbase, &dbkey, 0);

On the other hand opennhrp, needs the NUD_PROBE app queries to confirm
the existing entries, as opennhrp intentionally turns UCAST_PROBES to
zero.

So I'll submit the first variant soon, where app probes are added to be
done for NUD_PROBE.

- Timo

[PATCH net-next] neigh: probe application via netlink in NUD_PROBE

[Timo Teräs]

iproute2 arpd seems to expect this as there's code and comments
to handle netlink probes with NUD_PROBE set. It is used to flush
the arpd cached mappings.

opennhrp instead turns off unicast probes (so it can handle all
neighbour discovery). Without this change it will not see NUD_PROBE
probes and cannot reconfirm the mapping. Thus currently neigh entry
will just fail and can cause few packets dropped until broadcast
discovery is restarted.

Earlier discussion on the subject:
http://marc.info/?t=139305877100001&r=1&w=2

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
 net/core/neighbour.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index b9e9e0d..36d3f8c 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -836,10 +836,10 @@ out:
 static __inline__ int neigh_max_probes(struct neighbour *n)
 {
 	struct neigh_parms *p = n->parms;
-	return (n->nud_state & NUD_PROBE) ?
-		NEIGH_VAR(p, UCAST_PROBES) :
-		NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES) +
-		NEIGH_VAR(p, MCAST_PROBES);
+	int max_probes = NEIGH_VAR(p, UCAST_PROBES) + NEIGH_VAR(p, APP_PROBES);
+	if (!(n->nud_state & NUD_PROBE))
+		max_probes += NEIGH_VAR(p, MCAST_PROBES);
+	return max_probes;
 }
 
 static void neigh_invalidate(struct neighbour *neigh)
-- 
1.9.0

Re: [PATCH net-next] neigh: probe application via netlink in NUD_PROBE

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Wed, 26 Feb 2014 11:43:04 +0200

> iproute2 arpd seems to expect this as there's code and comments
> to handle netlink probes with NUD_PROBE set. It is used to flush
> the arpd cached mappings.
> 
> opennhrp instead turns off unicast probes (so it can handle all
> neighbour discovery). Without this change it will not see NUD_PROBE
> probes and cannot reconfirm the mapping. Thus currently neigh entry
> will just fail and can cause few packets dropped until broadcast
> discovery is restarted.
> 
> Earlier discussion on the subject:
> http://marc.info/?t=139305877100001&r=1&w=2
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

Applied, thanks a lot Timo.