[PATCH net-next] Better handling of transition to NUD_PROBE state

[PATCH net-next] Better handling of transition to NUD_PROBE state

[Erik Kline]

[1] When entering NUD_PROBE state via neigh_update(), perhaps received
    from userspace, correctly (re)initialize the probes count to zero.

    This is useful for forcing revalidation of a neighbor (for example
    if the host is attempting to do DNA [IPv4 4436, IPv6 6059]).

[2] Notify listeners when a neighbor goes into NUD_PROBE state.

    By sending notifications on entry to NUD_PROBE state listeners get
    more timely warnings of imminent connectivity issues.

    The current notifications on entry to NUD_STALE have somewhat
    limited usefulness: NUD_STALE is a perfectly normal state, as is
    NUD_DELAY, whereas notifications on entry to NUD_FAILURE come after
    a neighbor reachability problem has been confirmed (typically after
    three probes).

Signed-off-by: Erik Kline <ek@google.com>
---
 net/core/neighbour.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3de6542..3a74df7 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -913,6 +913,7 @@ static void neigh_timer_handler(unsigned long arg)
 			neigh->nud_state = NUD_PROBE;
 			neigh->updated = jiffies;
 			atomic_set(&neigh->probes, 0);
+			notify = 1;
 			next = now + NEIGH_VAR(neigh->parms, RETRANS_TIME);
 		}
 	} else {
@@ -1144,6 +1145,8 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 
 	if (new != old) {
 		neigh_del_timer(neigh);
+		if (new & NUD_PROBE)
+			atomic_set(&neigh->probes, 0);
 		if (new & NUD_IN_TIMER)
 			neigh_add_timer(neigh, (jiffies +
 						((new & NUD_REACHABLE) ?
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net-next] Better handling of transition to NUD_PROBE state

[Lorenzo Colitti]

On Mon, May 18, 2015 at 7:44 PM, Erik Kline <ek@google.com> wrote:
> [1] When entering NUD_PROBE state via neigh_update(), perhaps received
>     from userspace, correctly (re)initialize the probes count to zero.
>
>     This is useful for forcing revalidation of a neighbor (for example
>     if the host is attempting to do DNA [IPv4 4436, IPv6 6059]).
>
> [2] Notify listeners when a neighbor goes into NUD_PROBE state.
>
>     By sending notifications on entry to NUD_PROBE state listeners get
>     more timely warnings of imminent connectivity issues.
>
>     The current notifications on entry to NUD_STALE have somewhat
>     limited usefulness: NUD_STALE is a perfectly normal state, as is
>     NUD_DELAY, whereas notifications on entry to NUD_FAILURE come after
>     a neighbor reachability problem has been confirmed (typically after
>     three probes).
>
> Signed-off-by: Erik Kline <ek@google.com>
> ---
>  net/core/neighbour.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3de6542..3a74df7 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -913,6 +913,7 @@ static void neigh_timer_handler(unsigned long arg)
>                         neigh->nud_state = NUD_PROBE;
>                         neigh->updated = jiffies;
>                         atomic_set(&neigh->probes, 0);
> +                       notify = 1;

+1. Currently, the code notifies when going from REACHABLE into STALE,
which is not necessarily something userspace might want to know about
(all it means is "we haven't sent any packets to this neighbour
recently"), but it doesn't notify when going into PROBE, which is a
more important event (it means "we've been sending this neighbour
packets for (by default) 5 seconds and we still haven't found out if
it's stilll there, so we're probing it").

>                         next = now + NEIGH_VAR(neigh->parms, RETRANS_TIME);
>                 }
>         } else {
> @@ -1144,6 +1145,8 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
>
>         if (new != old) {
>                 neigh_del_timer(neigh);
> +               if (new & NUD_PROBE)
> +                       atomic_set(&neigh->probes, 0);

+1. The normal code path (from STALE to DELAY to PROBE) obviously
already sets the probes to 0. Userspace can put a neighbour into
NUD_PROBE to cause the kernel to probe it, but this only works (by
default) three times because the probe counter is never reset to 0. So
the first three times, the neighbour goes from (say) STALE to PROBE
and then back to REACHABLE (good), but then the fourth time, the
neighbour goes from STALE to PROBE and then immediately to FAILED.

>                 if (new & NUD_IN_TIMER)
>                         neigh_add_timer(neigh, (jiffies +
>                                                 ((new & NUD_REACHABLE) ?
> --
> 2.2.0.rc0.207.ga3a616c
>

Acked-By: Lorenzo Colitti <lorenzo@google.com>

Re: [PATCH net-next] Better handling of transition to NUD_PROBE state

[Hannes Frederic Sowa]

On Mi, 2015-05-20 at 12:42 +0900, Lorenzo Colitti wrote:
> On Mon, May 18, 2015 at 7:44 PM, Erik Kline <ek@google.com> wrote:
> > [1] When entering NUD_PROBE state via neigh_update(), perhaps received
> >     from userspace, correctly (re)initialize the probes count to zero.
> >
> >     This is useful for forcing revalidation of a neighbor (for example
> >     if the host is attempting to do DNA [IPv4 4436, IPv6 6059]).
> >
> > [2] Notify listeners when a neighbor goes into NUD_PROBE state.
> >
> >     By sending notifications on entry to NUD_PROBE state listeners get
> >     more timely warnings of imminent connectivity issues.
> >
> >     The current notifications on entry to NUD_STALE have somewhat
> >     limited usefulness: NUD_STALE is a perfectly normal state, as is
> >     NUD_DELAY, whereas notifications on entry to NUD_FAILURE come after
> >     a neighbor reachability problem has been confirmed (typically after
> >     three probes).
> >
> > Signed-off-by: Erik Kline <ek@google.com>
> > ---
> >  net/core/neighbour.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 3de6542..3a74df7 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -913,6 +913,7 @@ static void neigh_timer_handler(unsigned long arg)
> >                         neigh->nud_state = NUD_PROBE;
> >                         neigh->updated = jiffies;
> >                         atomic_set(&neigh->probes, 0);
> > +                       notify = 1;
> 
> +1. Currently, the code notifies when going from REACHABLE into STALE,
> which is not necessarily something userspace might want to know about
> (all it means is "we haven't sent any packets to this neighbour
> recently"), but it doesn't notify when going into PROBE, which is a
> more important event (it means "we've been sending this neighbour
> packets for (by default) 5 seconds and we still haven't found out if
> it's stilll there, so we're probing it").
> 
> >                         next = now + NEIGH_VAR(neigh->parms, RETRANS_TIME);
> >                 }
> >         } else {
> > @@ -1144,6 +1145,8 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
> >
> >         if (new != old) {
> >                 neigh_del_timer(neigh);
> > +               if (new & NUD_PROBE)
> > +                       atomic_set(&neigh->probes, 0);
> 
> +1. The normal code path (from STALE to DELAY to PROBE) obviously
> already sets the probes to 0. Userspace can put a neighbour into
> NUD_PROBE to cause the kernel to probe it, but this only works (by
> default) three times because the probe counter is never reset to 0. So
> the first three times, the neighbour goes from (say) STALE to PROBE
> and then back to REACHABLE (good), but then the fourth time, the
> neighbour goes from STALE to PROBE and then immediately to FAILED.
> 
> >                 if (new & NUD_IN_TIMER)
> >                         neigh_add_timer(neigh, (jiffies +
> >                                                 ((new & NUD_REACHABLE) ?
> > --
> > 2.2.0.rc0.207.ga3a616c
> >
> 
> Acked-By: Lorenzo Colitti <lorenzo@google.com>

I agree with Lorenzo, these changes look fine.

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net-next] Better handling of transition to NUD_PROBE state

[David Miller]

From: Erik Kline <ek@google.com>
Date: Mon, 18 May 2015 19:44:41 +0900

> [1] When entering NUD_PROBE state via neigh_update(), perhaps received
>     from userspace, correctly (re)initialize the probes count to zero.
> 
>     This is useful for forcing revalidation of a neighbor (for example
>     if the host is attempting to do DNA [IPv4 4436, IPv6 6059]).
> 
> [2] Notify listeners when a neighbor goes into NUD_PROBE state.
> 
>     By sending notifications on entry to NUD_PROBE state listeners get
>     more timely warnings of imminent connectivity issues.
> 
>     The current notifications on entry to NUD_STALE have somewhat
>     limited usefulness: NUD_STALE is a perfectly normal state, as is
>     NUD_DELAY, whereas notifications on entry to NUD_FAILURE come after
>     a neighbor reachability problem has been confirmed (typically after
>     three probes).
> 
> Signed-off-by: Erik Kline <ek@google.com>

Please, in the future, put a proper subsystem prefix in the Subject
line of your patch submissions.  In this particular case "neigh: "
would have been appropriate and is what I added when applying your
patch.

Applied, thanks.

Re: [PATCH net-next] Better handling of transition to NUD_PROBE state

[Erik Kline]

> Please, in the future, put a proper subsystem prefix in the Subject
> line of your patch submissions.  In this particular case "neigh: "
> would have been appropriate and is what I added when applying your
> patch.

My apologies.

> Applied, thanks.

Appreciated.
-ek