[PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Dmitry Popov]

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 <ixaphire@qrator.net>
---
 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;

Re: [PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Dmitry Popov]

On Fri, 6 Jun 2014 22:51:17 +0400
Dmitry Popov <ixaphire@qrator.net> wrote:

> ipv4_{update_pmtu,redirect) were called with tunnel's ifindex (t->dev is a 

Should be read as 
ipv4_{update_pmtu,redirect} ...

I will resubmit the patch, sorry. :(

[PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Dmitry Popov]

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 <ixaphire@qrator.net>
---
 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;

Re: [PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Cong Wang]

On Fri, Jun 6, 2014 at 12:19 PM, Dmitry Popov <ixaphire@qrator.net> 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 <ixaphire@qrator.net>
> ---
>  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?

Re: [PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Dmitry Popov]

On Fri, 6 Jun 2014 17:45:44 -0700
Cong Wang <cwang@twopensource.com> 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.

Re: [PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[Dmitry Popov]

On Sat, 7 Jun 2014 19:54:12 +0400
Dmitry Popov <ixaphire@qrator.net> 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.

Re: [PATCH] ipip, sit: fix ipv4_{update_pmtu,redirect} calls

[David Miller]

From: Dmitry Popov <ixaphire@qrator.net>
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 <ixaphire@qrator.net>

Applied and queued up for -stable, thanks.