ipv4_{update_pmtu,redirect} were called with tunnel's ifindex (t->dev is a
tunnel netdevice). It caused wrong route lookup and failure of pmtu update or
redirect. We should use the same ifindex that we use in ip_route_output_* in
*tunnel_xmit code. It is t->parms.link .
Signed-off-by: Dmitry Popov <[email protected]>
---
net/ipv4/ipip.c | 4 ++--
net/ipv6/sit.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 812b183..09680dd 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -149,13 +149,13 @@ static int ipip_err(struct sk_buff *skb, u32 info)
if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
ipv4_update_pmtu(skb, dev_net(skb->dev), info,
- t->dev->ifindex, 0, IPPROTO_IPIP, 0);
+ t->parms.link, 0, IPPROTO_IPIP, 0);
err = 0;
goto out;
}
if (type == ICMP_REDIRECT) {
- ipv4_redirect(skb, dev_net(skb->dev), t->dev->ifindex, 0,
+ ipv4_redirect(skb, dev_net(skb->dev), t->parms.link, 0,
IPPROTO_IPIP, 0);
err = 0;
goto out;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index e5a453c..45397b2 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -560,12 +560,12 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
ipv4_update_pmtu(skb, dev_net(skb->dev), info,
- t->dev->ifindex, 0, IPPROTO_IPV6, 0);
+ t->parms.link, 0, IPPROTO_IPV6, 0);
err = 0;
goto out;
}
if (type == ICMP_REDIRECT) {
- ipv4_redirect(skb, dev_net(skb->dev), t->dev->ifindex, 0,
+ ipv4_redirect(skb, dev_net(skb->dev), t->parms.link, 0,
IPPROTO_IPV6, 0);
err = 0;
goto out;
On Fri, Jun 6, 2014 at 12:19 PM, Dmitry Popov <[email protected]> wrote:
> ipv4_{update_pmtu,redirect} were called with tunnel's ifindex (t->dev is a
> tunnel netdevice). It caused wrong route lookup and failure of pmtu update or
> redirect. We should use the same ifindex that we use in ip_route_output_* in
> *tunnel_xmit code. It is t->parms.link .
>
> Signed-off-by: Dmitry Popov <[email protected]>
> ---
> net/ipv4/ipip.c | 4 ++--
> net/ipv6/sit.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
I think vti (v4 and v6) tunnel needs to fix as well?
On Fri, 6 Jun 2014 17:45:44 -0700
Cong Wang <[email protected]> wrote:
> I think vti (v4 and v6) tunnel needs to fix as well?
1) vti:
Well, probably it needs a fix, but:
a) I don't know a scenario when vti4_err would be called, ah4/esp4/ipcomp4
should handle icmp errors. vti_err was removed in aba826958830 but (don't know
why, maybe someone can explain?) vti4_err was introduced back in df3893c176e9.
b) vti uses complex routing through xfrm, see pt. 2).
2) ah4/esp/ipcomp:
They probably have a problem with ipv4_{update_pmtu,redirect} too, because
ah4/esp/ipcomp use xfrm policies for correct routing, but ipv4_{update_pmtu,
redirect} implementation do not xfrm_lookup. Moreover, the whole
ipv4_*_{update_pmtu, redirect} family looks like using __ip_route_output_key
which doesn't do xfrm_lookup, except ipv4_sk_update_pmtu which does it through
ip_route_output_flow. I also do not understand ideas behind it, but I am not
so familiar with xfrm stuff, so feel free to correct me.
Also ah4/esp/ipcomp use oif=0 for ipv4_{update_pmtu,redirect} which should work
well with most setups (and wrong oif !=0 like in ipip/sit doesn't work), so I
don't think it's a big problem.
3) gre:
There are 2 gre protocols in kernel: pptp(drivers/net/ppp/pptp.c) and
cisco/ipgre(net/ipv4/gre_demux.c).
Pptp simply ignores ICMP errors, I'd simply let it be.
ipgre is a framework for subprotos which doesn't work with tunnel devices by
itself (see net/ipv4/gre_demux.c:gre_cisco_err). Although it uses
skb->dev->ifindex for ipv4_{update_pmtu,redirect} which might be wrong for hosts
with asymmetric routing, this is not a big deal, because tunnels bound to device
will not work with asymmetric routing anyway. So I think it is okay.
On Sat, 7 Jun 2014 19:54:12 +0400
Dmitry Popov <[email protected]> wrote:
> 3) gre:
> ipgre is a framework for subprotos which doesn't work with tunnel devices by
> itself (see net/ipv4/gre_demux.c:gre_cisco_err). Although it uses
> skb->dev->ifindex for ipv4_{update_pmtu,redirect} which might be wrong for hosts
> with asymmetric routing, this is not a big deal, because tunnels bound to device
> will not work with asymmetric routing anyway. So I think it is okay.
Actually, yes, it may not work in case of unbound tunnel and asymmetric routing,
but we'll need to put icmp redirects/frag_needed handling inside
gre_cisco_protocol->err_handler then, I am not sure if it's worth it.
From: Dmitry Popov <[email protected]>
Date: Fri, 6 Jun 2014 23:19:21 +0400
> ipv4_{update_pmtu,redirect} were called with tunnel's ifindex (t->dev is a
> tunnel netdevice). It caused wrong route lookup and failure of pmtu update or
> redirect. We should use the same ifindex that we use in ip_route_output_* in
> *tunnel_xmit code. It is t->parms.link .
>
> Signed-off-by: Dmitry Popov <[email protected]>
Applied and queued up for -stable, thanks.