* About caching unreachable routes when not forwarding
@ 2014-09-12 14:14 Nicolas Cavallari
2014-09-12 14:14 ` [RFC] ipv4: Do not cache routing failures due to disabled forwarding Nicolas Cavallari
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Cavallari @ 2014-09-12 14:14 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
I have some weird routing problem on a seemingly simple setup on a
3.12-3.16 kernel and I suspect that net-next is also affected.
I have two interfaces: A, B, with forwarding disabled on A and
enabled on B. I also have another interface V.
Both interfaces receive a packet that must be routed though V:
- A receives the packet first, do some fib lookup and cache a "route
unreachable" rt_iif=0 because forwarding is disabled.
- B receives the packet, do some fib lookup (and reverse path filtering, whatever) and succeeds, then finds the recently cached rth and use it. This cached rth is of course "route unreachable" and forwarding is broken.
This simple script in a network namespace is enough to show it:
#!/bin/sh -eu
setup_iface () {
ip link add name "$1" type dummy
ip link set dev "$1" up
[ -n "${2:-}" ] && ip route add "$2" dev "$1"
ip4conf="/proc/sys/net/ipv4/conf/$1"
echo "$3" > "$ip4conf/forwarding"
echo "$4" > "$ip4conf/rp_filter"
}
ip link set lo up
setup_iface "A" "" 0 0
setup_iface "B" "10.0.0.2/32" 1 1
setup_iface "C" "10.0.0.1/32" 1 0
set -x +e
ip route get 10.0.0.1 from 10.0.0.2 iif A
# unreachable
ip route get 10.0.0.1 from 10.0.0.2 iif B
# unreachable, but should be reachable
ip route flush cache
ip route get 10.0.0.1 from 10.0.0.2 iif B
# reachable
ip route get 10.0.0.1 from 10.0.0.2 iif A
# reachable, but should be unreachable
I would suggest that we shouldn't cache unreachable results due to
forwarding being disabled, but I'm not aware of all use-case of this
code nor what should actually be cached. The following patch fix
my use case. I don't know if it breaks others.
^ permalink raw reply [flat|nested] 6+ messages in thread* [RFC] ipv4: Do not cache routing failures due to disabled forwarding. 2014-09-12 14:14 About caching unreachable routes when not forwarding Nicolas Cavallari @ 2014-09-12 14:14 ` Nicolas Cavallari 2014-09-12 22:13 ` Julian Anastasov 2014-10-29 19:03 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Nicolas Cavallari @ 2014-09-12 14:14 UTC (permalink / raw) To: netdev Cc: David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy If we cache them, the kernel will reuse them, independently of whether forwarding is enabled or not. Which means that if forwarding is disabled on the input interface where the first routing request comes from, then that unreachable result will be cached and reused for other interfaces, even if forwarding is enabled on them. This can be verified with two interfaces A and B and an output interface C, where B has forwarding enabled, but not A and trying ip route get $dst iif A from $src && ip route get $dst iif B from $src Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> --- based on net-next, but not really tested on top of it. net/ipv4/route.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 234a43e..b537997 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1655,7 +1655,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, struct rtable *rth; int err = -EINVAL; struct net *net = dev_net(dev); - bool do_cache; + bool do_cache = true; /* IP on this device is disabled. */ @@ -1723,6 +1723,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, if (!IN_DEV_FORWARD(in_dev)) { err = -EHOSTUNREACH; + do_cache = false; goto no_route; } if (res.type != RTN_UNICAST) @@ -1746,16 +1747,14 @@ brd_input: RT_CACHE_STAT_INC(in_brd); local_input: - do_cache = false; - if (res.fi) { - if (!itag) { - rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input); - if (rt_cache_valid(rth)) { - skb_dst_set_noref(skb, &rth->dst); - err = 0; - goto out; - } - do_cache = true; + if (!res.fi || itag) { + do_cache = false; + } else if (do_cache) { + rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input); + if (rt_cache_valid(rth)) { + skb_dst_set_noref(skb, &rth->dst); + err = 0; + goto out; } } -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] ipv4: Do not cache routing failures due to disabled forwarding. 2014-09-12 14:14 ` [RFC] ipv4: Do not cache routing failures due to disabled forwarding Nicolas Cavallari @ 2014-09-12 22:13 ` Julian Anastasov 2014-10-29 19:03 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: Julian Anastasov @ 2014-09-12 22:13 UTC (permalink / raw) To: Nicolas Cavallari Cc: netdev, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy Hello, On Fri, 12 Sep 2014, Nicolas Cavallari wrote: > If we cache them, the kernel will reuse them, independently of > whether forwarding is enabled or not. Which means that if forwarding is > disabled on the input interface where the first routing request comes > from, then that unreachable result will be cached and reused for > other interfaces, even if forwarding is enabled on them. > > This can be verified with two interfaces A and B and an output interface > C, where B has forwarding enabled, but not A and trying > ip route get $dst iif A from $src && ip route get $dst iif B from $src Correct. While failed fib_lookup() does not set res.fi in net/ipv4/fib_trie.c:check_leaf(), on fib_lookup() success we have res.fi != NULL and it remains for the !IN_DEV_FORWARD case (the 2nd 'goto no_route'). > Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> > --- > based on net-next, but not really tested on top of it. > > net/ipv4/route.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index 234a43e..b537997 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -1655,7 +1655,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, > struct rtable *rth; > int err = -EINVAL; > struct net *net = dev_net(dev); > - bool do_cache; > + bool do_cache = true; > > /* IP on this device is disabled. */ > > @@ -1723,6 +1723,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, > > if (!IN_DEV_FORWARD(in_dev)) { > err = -EHOSTUNREACH; > + do_cache = false; > goto no_route; > } > if (res.type != RTN_UNICAST) > @@ -1746,16 +1747,14 @@ brd_input: > RT_CACHE_STAT_INC(in_brd); > > local_input: > - do_cache = false; > - if (res.fi) { > - if (!itag) { > - rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input); > - if (rt_cache_valid(rth)) { > - skb_dst_set_noref(skb, &rth->dst); > - err = 0; > - goto out; > - } > - do_cache = true; > + if (!res.fi || itag) { > + do_cache = false; > + } else if (do_cache) { > + rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input); > + if (rt_cache_valid(rth)) { > + skb_dst_set_noref(skb, &rth->dst); > + err = 0; > + goto out; > } > } > > -- > 2.1.0 Two alternatives are possible: 1. set res.fi = NULL after 'no_route:' label or better 2. set do_cache = false after 'no_route:' label, then instead of 'goto local_input;' jump to a new label 'create_rt:' just before rt_dst_alloc. Not sure, they may generate less code in the fast path. Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] ipv4: Do not cache routing failures due to disabled forwarding. 2014-09-12 14:14 ` [RFC] ipv4: Do not cache routing failures due to disabled forwarding Nicolas Cavallari 2014-09-12 22:13 ` Julian Anastasov @ 2014-10-29 19:03 ` David Miller 2014-10-30 9:09 ` [PATCH RESEND v2] " Nicolas Cavallari 1 sibling, 1 reply; 6+ messages in thread From: David Miller @ 2014-10-29 19:03 UTC (permalink / raw) To: nicolas.cavallari; +Cc: netdev, kuznet, jmorris, yoshfuji, kaber From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> Date: Fri, 12 Sep 2014 16:14:20 +0200 > If we cache them, the kernel will reuse them, independently of > whether forwarding is enabled or not. Which means that if forwarding is > disabled on the input interface where the first routing request comes > from, then that unreachable result will be cached and reused for > other interfaces, even if forwarding is enabled on them. > > This can be verified with two interfaces A and B and an output interface > C, where B has forwarding enabled, but not A and trying > ip route get $dst iif A from $src && ip route get $dst iif B from $src > > Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> > --- > based on net-next, but not really tested on top of it. Sorry Nicolas, this seems to have fallen on the floor. Could you please resubmit your most uptodate version of this patch so I can apply it? Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RESEND v2] ipv4: Do not cache routing failures due to disabled forwarding. 2014-10-29 19:03 ` David Miller @ 2014-10-30 9:09 ` Nicolas Cavallari 2014-10-30 23:21 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Nicolas Cavallari @ 2014-10-30 9:09 UTC (permalink / raw) To: David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy Cc: netdev, linux-kernel If we cache them, the kernel will reuse them, independently of whether forwarding is enabled or not. Which means that if forwarding is disabled on the input interface where the first routing request comes from, then that unreachable result will be cached and reused for other interfaces, even if forwarding is enabled on them. The opposite is also true. This can be verified with two interfaces A and B and an output interface C, where B has forwarding enabled, but not A and trying ip route get $dst iif A from $src && ip route get $dst iif B from $src Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> Reviewed-by: Julian Anastasov <ja@ssi.bg> --- > Sorry Nicolas, this seems to have fallen on the floor. Could you please > resubmit your most uptodate version of this patch so I can apply it? Here you are. diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 2d4ae46..6a2155b 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1798,6 +1798,7 @@ local_input: no_route: RT_CACHE_STAT_INC(in_no_route); res.type = RTN_UNREACHABLE; + res.fi = NULL; goto local_input; /* -- 2.1.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v2] ipv4: Do not cache routing failures due to disabled forwarding. 2014-10-30 9:09 ` [PATCH RESEND v2] " Nicolas Cavallari @ 2014-10-30 23:21 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2014-10-30 23:21 UTC (permalink / raw) To: nicolas.cavallari; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> Date: Thu, 30 Oct 2014 10:09:53 +0100 > If we cache them, the kernel will reuse them, independently of > whether forwarding is enabled or not. Which means that if forwarding is > disabled on the input interface where the first routing request comes > from, then that unreachable result will be cached and reused for > other interfaces, even if forwarding is enabled on them. The opposite > is also true. > > This can be verified with two interfaces A and B and an output interface > C, where B has forwarding enabled, but not A and trying > ip route get $dst iif A from $src && ip route get $dst iif B from $src > > Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> > Reviewed-by: Julian Anastasov <ja@ssi.bg> Applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-30 23:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-12 14:14 About caching unreachable routes when not forwarding Nicolas Cavallari 2014-09-12 14:14 ` [RFC] ipv4: Do not cache routing failures due to disabled forwarding Nicolas Cavallari 2014-09-12 22:13 ` Julian Anastasov 2014-10-29 19:03 ` David Miller 2014-10-30 9:09 ` [PATCH RESEND v2] " Nicolas Cavallari 2014-10-30 23:21 ` David Miller
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).