* [PATCH net-next] ipv6: router reachability probing
@ 2013-12-10 16:15 Jiri Benc
2013-12-10 20:51 ` Hannes Frederic Sowa
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Benc @ 2013-12-10 16:15 UTC (permalink / raw)
To: netdev; +Cc: Hannes Frederic Sowa, Hideaki YOSHIFUJI
RFC 4191 states in 3.5:
When a host avoids using any non-reachable router X and instead sends
a data packet to another router Y, and the host would have used
router X if router X were reachable, then the host SHOULD probe each
such router X's reachability by sending a single Neighbor
Solicitation to that router's address. A host MUST NOT probe a
router's reachability in the absence of useful traffic that the host
would have sent to the router if it were reachable. In any case,
these probes MUST be rate-limited to no more than one per minute per
router.
Currently, when the neighbour corresponding to a router falls into
NUD_FAILED, it's never considered again. Introduce a new rt6_nud_state
value, RT6_NUD_FAIL_PROBE, which suggests the route should not be used but
should be probed with a single NS. The probe is ratelimited by the existing
code. To better distinguish meanings of the failure values, rename
RT6_NUD_FAIL_SOFT to RT6_NUD_FAIL_DO_RR.
We also need to accept NAs when in NUD_FAILED state.
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
I admit I'm not much sure about the removal of the NUD_FAILED check in
ndisc_recv_na. I can't find anything in RFC 4861 nor in RFC 4191 which would
prohibit taking the neighbor entry out of failed state if a NA is received.
I also cannot find any place where such change would cause problems in the
kernel, but I may have easily overlooked something. Comments are welcome.
---
net/ipv6/ndisc.c | 3 ---
net/ipv6/route.c | 14 +++++++++-----
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 09a22f4f36c9..faf67efc4c99 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -918,9 +918,6 @@ static void ndisc_recv_na(struct sk_buff *skb)
u8 old_flags = neigh->flags;
struct net *net = dev_net(dev);
- if (neigh->nud_state & NUD_FAILED)
- goto out;
-
/*
* Don't update the neighbor cache entry on a proxy NA from
* ourselves because either the proxied node is off link or it
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ddb9d41c8eea..147c39f7f70d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -66,8 +66,9 @@
#endif
enum rt6_nud_state {
- RT6_NUD_FAIL_HARD = -2,
- RT6_NUD_FAIL_SOFT = -1,
+ RT6_NUD_FAIL_HARD = -3,
+ RT6_NUD_FAIL_PROBE = -2,
+ RT6_NUD_FAIL_DO_RR = -1,
RT6_NUD_SUCCEED = 1
};
@@ -577,11 +578,13 @@ static inline enum rt6_nud_state rt6_check_neigh(struct rt6_info *rt)
#ifdef CONFIG_IPV6_ROUTER_PREF
else if (!(neigh->nud_state & NUD_FAILED))
ret = RT6_NUD_SUCCEED;
+ else
+ ret = RT6_NUD_FAIL_PROBE;
#endif
read_unlock(&neigh->lock);
} else {
ret = IS_ENABLED(CONFIG_IPV6_ROUTER_PREF) ?
- RT6_NUD_SUCCEED : RT6_NUD_FAIL_SOFT;
+ RT6_NUD_SUCCEED : RT6_NUD_FAIL_DO_RR;
}
rcu_read_unlock_bh();
@@ -618,16 +621,17 @@ static struct rt6_info *find_match(struct rt6_info *rt, int oif, int strict,
goto out;
m = rt6_score_route(rt, oif, strict);
- if (m == RT6_NUD_FAIL_SOFT) {
+ if (m == RT6_NUD_FAIL_DO_RR) {
match_do_rr = true;
m = 0; /* lowest valid score */
- } else if (m < 0) {
+ } else if (m == RT6_NUD_FAIL_HARD) {
goto out;
}
if (strict & RT6_LOOKUP_F_REACHABLE)
rt6_probe(rt);
+ /* note that m can be RT6_NUD_FAIL_PROBE at this point */
if (m > *mpri) {
*do_rr = match_do_rr;
*mpri = m;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next] ipv6: router reachability probing
2013-12-10 16:15 [PATCH net-next] ipv6: router reachability probing Jiri Benc
@ 2013-12-10 20:51 ` Hannes Frederic Sowa
2013-12-10 21:01 ` Jiri Benc
0 siblings, 1 reply; 3+ messages in thread
From: Hannes Frederic Sowa @ 2013-12-10 20:51 UTC (permalink / raw)
To: Jiri Benc; +Cc: netdev, Hideaki YOSHIFUJI
On Tue, Dec 10, 2013 at 05:15:44PM +0100, Jiri Benc wrote:
> RFC 4191 states in 3.5:
>
> When a host avoids using any non-reachable router X and instead sends
> a data packet to another router Y, and the host would have used
> router X if router X were reachable, then the host SHOULD probe each
> such router X's reachability by sending a single Neighbor
> Solicitation to that router's address. A host MUST NOT probe a
> router's reachability in the absence of useful traffic that the host
> would have sent to the router if it were reachable. In any case,
> these probes MUST be rate-limited to no more than one per minute per
> router.
>
> Currently, when the neighbour corresponding to a router falls into
> NUD_FAILED, it's never considered again. Introduce a new rt6_nud_state
> value, RT6_NUD_FAIL_PROBE, which suggests the route should not be used but
> should be probed with a single NS. The probe is ratelimited by the existing
> code. To better distinguish meanings of the failure values, rename
> RT6_NUD_FAIL_SOFT to RT6_NUD_FAIL_DO_RR.
>
> We also need to accept NAs when in NUD_FAILED state.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> I admit I'm not much sure about the removal of the NUD_FAILED check in
> ndisc_recv_na. I can't find anything in RFC 4861 nor in RFC 4191 which would
> prohibit taking the neighbor entry out of failed state if a NA is received.
> I also cannot find any place where such change would cause problems in the
> kernel, but I may have easily overlooked something. Comments are welcome.
> ---
> net/ipv6/ndisc.c | 3 ---
> net/ipv6/route.c | 14 +++++++++-----
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 09a22f4f36c9..faf67efc4c99 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -918,9 +918,6 @@ static void ndisc_recv_na(struct sk_buff *skb)
> u8 old_flags = neigh->flags;
> struct net *net = dev_net(dev);
>
> - if (neigh->nud_state & NUD_FAILED)
> - goto out;
> -
> /*
> * Don't update the neighbor cache entry on a proxy NA from
> * ourselves because either the proxied node is off link or it
The reason is that we don't send a neighbour advertisment in FAILED state
and thus shouldn't accept one. RFC 4861 7.2.5. specifies that we should
not add a new entry to the neighbour discovery table just because of a
neighbour advertisment. Because NUD_FAILED is just an artifical state
to the linux kernel we should treat it like the entry is not existent.
The canonical way would be to place this neighbor in the NUD_PROBE state
in rt6_probe.
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index ddb9d41c8eea..147c39f7f70d 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -66,8 +66,9 @@
> #endif
>
> enum rt6_nud_state {
> - RT6_NUD_FAIL_HARD = -2,
> - RT6_NUD_FAIL_SOFT = -1,
> + RT6_NUD_FAIL_HARD = -3,
> + RT6_NUD_FAIL_PROBE = -2,
> + RT6_NUD_FAIL_DO_RR = -1,
> RT6_NUD_SUCCEED = 1
> };
>
> @@ -577,11 +578,13 @@ static inline enum rt6_nud_state rt6_check_neigh(struct rt6_info *rt)
> #ifdef CONFIG_IPV6_ROUTER_PREF
> else if (!(neigh->nud_state & NUD_FAILED))
> ret = RT6_NUD_SUCCEED;
> + else
> + ret = RT6_NUD_FAIL_PROBE;
> #endif
Nit:
We could now change the declaration of ret in rt6_check_neigh from
"enum rt6_nud_state ret = RT6_NUD_FAIL_HARD"
to
"enum rt6_nud_state ret;" as all cases are covered now.
Otherwise the patch is a nice improvment.
Thanks,
Hannes
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] ipv6: router reachability probing
2013-12-10 20:51 ` Hannes Frederic Sowa
@ 2013-12-10 21:01 ` Jiri Benc
0 siblings, 0 replies; 3+ messages in thread
From: Jiri Benc @ 2013-12-10 21:01 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev, Hideaki YOSHIFUJI
On Tue, 10 Dec 2013 21:51:01 +0100, Hannes Frederic Sowa wrote:
> The reason is that we don't send a neighbour advertisment in FAILED state
> and thus shouldn't accept one. RFC 4861 7.2.5. specifies that we should
> not add a new entry to the neighbour discovery table just because of a
> neighbour advertisment. Because NUD_FAILED is just an artifical state
> to the linux kernel we should treat it like the entry is not existent.
That was the missing piece, thanks!
> The canonical way would be to place this neighbor in the NUD_PROBE state
> in rt6_probe.
Okay, will respin the patch.
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index ddb9d41c8eea..147c39f7f70d 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -66,8 +66,9 @@
> > #endif
> >
> > enum rt6_nud_state {
> > - RT6_NUD_FAIL_HARD = -2,
> > - RT6_NUD_FAIL_SOFT = -1,
> > + RT6_NUD_FAIL_HARD = -3,
> > + RT6_NUD_FAIL_PROBE = -2,
> > + RT6_NUD_FAIL_DO_RR = -1,
> > RT6_NUD_SUCCEED = 1
> > };
> >
> > @@ -577,11 +578,13 @@ static inline enum rt6_nud_state rt6_check_neigh(struct rt6_info *rt)
> > #ifdef CONFIG_IPV6_ROUTER_PREF
> > else if (!(neigh->nud_state & NUD_FAILED))
> > ret = RT6_NUD_SUCCEED;
> > + else
> > + ret = RT6_NUD_FAIL_PROBE;
> > #endif
>
> Nit:
> We could now change the declaration of ret in rt6_check_neigh from
> "enum rt6_nud_state ret = RT6_NUD_FAIL_HARD"
> to
> "enum rt6_nud_state ret;" as all cases are covered now.
That's what I originally thought, too, but it's not the case when
CONFIG_IPV6_ROUTER_PREF is not defined and neigh->nud_state != NUD_VALID.
> Otherwise the patch is a nice improvment.
Thanks!
Jiri
--
Jiri Benc
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-12-10 21:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-10 16:15 [PATCH net-next] ipv6: router reachability probing Jiri Benc
2013-12-10 20:51 ` Hannes Frederic Sowa
2013-12-10 21:01 ` Jiri Benc
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).