[PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Haishuang Yan]

When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
skb_dst_set to begin and tun_dst would be freed by kfree_skb.

CC: Pravin B Shelar <pshelar@nicira.com>
Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
---
 net/ipv4/ip_tunnel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index b878ecb..27fc20f 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
 	const struct iphdr *iph = ip_hdr(skb);
 	int err;
 
+	if (tun_dst)
+		skb_dst_set(skb, (struct dst_entry *)tun_dst);
+
 #ifdef CONFIG_NET_IPGRE_BROADCAST
 	if (ipv4_is_multicast(iph->daddr)) {
 		tunnel->dev->stats.multicast++;
@@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
 		skb->dev = tunnel->dev;
 	}
 
-	if (tun_dst)
-		skb_dst_set(skb, (struct dst_entry *)tun_dst);
-
 	gro_cells_receive(&tunnel->gro_cells, skb);
 	return 0;
 
-- 
1.8.3.1

[PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Haishuang Yan]

When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
skb_dst_set to begin and tun_dst would be freed by kfree_skb.

CC: Alexei Starovoitov <ast@fb.com>
Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 net/ipv6/ip6_tunnel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 9b37f97..bf45f1b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -789,6 +789,9 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
 	const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
 	int err;
 
+	if (tun_dst)
+		skb_dst_set(skb, (struct dst_entry *)tun_dst);
+
 	if ((!(tpi->flags & TUNNEL_CSUM) &&
 	     (tunnel->parms.i_flags & TUNNEL_CSUM)) ||
 	    ((tpi->flags & TUNNEL_CSUM) &&
@@ -852,9 +855,6 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
 
 	skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(tunnel->dev)));
 
-	if (tun_dst)
-		skb_dst_set(skb, (struct dst_entry *)tun_dst);
-
 	gro_cells_receive(&tunnel->gro_cells, skb);
 	return 0;
 
-- 
1.8.3.1

Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Pravin Shelar]

On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
<yanhaishuang@cmss.chinamobile.com> wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> ---
>  net/ipv4/ip_tunnel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index b878ecb..27fc20f 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>         const struct iphdr *iph = ip_hdr(skb);
>         int err;
>
> +       if (tun_dst)
> +               skb_dst_set(skb, (struct dst_entry *)tun_dst);
> +
If dst is set so early, skb_scrub_packet() would remove the tunnel dst
reference.
It is better to call skb_dst_drop() from error code path.

>  #ifdef CONFIG_NET_IPGRE_BROADCAST
>         if (ipv4_is_multicast(iph->daddr)) {
>                 tunnel->dev->stats.multicast++;
> @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>                 skb->dev = tunnel->dev;
>         }
>
> -       if (tun_dst)
> -               skb_dst_set(skb, (struct dst_entry *)tun_dst);
> -
>         gro_cells_receive(&tunnel->gro_cells, skb);
>         return 0;
>
> --
> 1.8.3.1
>
>
>

Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Eric Dumazet]

On Thu, 2017-06-08 at 08:57 +0800, Haishuang Yan wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
> 
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks a lot.

Re: [PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Eric Dumazet]

On Thu, 2017-06-08 at 08:57 +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
> 
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Alexei Starovoitov]

On Thu, Jun 08, 2017 at 08:57:56AM +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
> 
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

Good catch. Thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[严海双]

> On 8 Jun 2017, at 10:13 AM, Pravin Shelar <pshelar@ovn.org> wrote:
> 
> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
> <yanhaishuang@cmss.chinamobile.com> wrote:
>> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
>> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>> 
>> CC: Pravin B Shelar <pshelar@nicira.com>
>> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
>> ---
>> net/ipv4/ip_tunnel.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> index b878ecb..27fc20f 100644
>> --- a/net/ipv4/ip_tunnel.c
>> +++ b/net/ipv4/ip_tunnel.c
>> @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>>        const struct iphdr *iph = ip_hdr(skb);
>>        int err;
>> 
>> +       if (tun_dst)
>> +               skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> +
> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
> reference.
> It is better to call skb_dst_drop() from error code path.

Yes, I will change it in v3 commit, thanks!

> 
>> #ifdef CONFIG_NET_IPGRE_BROADCAST
>>        if (ipv4_is_multicast(iph->daddr)) {
>>                tunnel->dev->stats.multicast++;
>> @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>>                skb->dev = tunnel->dev;
>>        }
>> 
>> -       if (tun_dst)
>> -               skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> -
>>        gro_cells_receive(&tunnel->gro_cells, skb);
>>        return 0;
>> 
>> --
>> 1.8.3.1
>> 
>> 
>> 
> 

Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Eric Dumazet]

On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote:
> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
> <yanhaishuang@cmss.chinamobile.com> wrote:
> > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> > skb_dst_set to begin and tun_dst would be freed by kfree_skb.
> >
> > CC: Pravin B Shelar <pshelar@nicira.com>
> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> > ---
> >  net/ipv4/ip_tunnel.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> > index b878ecb..27fc20f 100644
> > --- a/net/ipv4/ip_tunnel.c
> > +++ b/net/ipv4/ip_tunnel.c
> > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
> >         const struct iphdr *iph = ip_hdr(skb);
> >         int err;
> >
> > +       if (tun_dst)
> > +               skb_dst_set(skb, (struct dst_entry *)tun_dst);
> > +
> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
> reference.
> It is better to call skb_dst_drop() from error code path.

Do we really want to keep a dst from another namespace if
skb_scrub_packet() is called with xnet=true ?

 

Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Pravin Shelar]

On Wed, Jun 7, 2017 at 8:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote:
>> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
>> <yanhaishuang@cmss.chinamobile.com> wrote:
>> > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
>> > skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>> >
>> > CC: Pravin B Shelar <pshelar@nicira.com>
>> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>> > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
>> > ---
>> >  net/ipv4/ip_tunnel.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> > index b878ecb..27fc20f 100644
>> > --- a/net/ipv4/ip_tunnel.c
>> > +++ b/net/ipv4/ip_tunnel.c
>> > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>> >         const struct iphdr *iph = ip_hdr(skb);
>> >         int err;
>> >
>> > +       if (tun_dst)
>> > +               skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> > +
>> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
>> reference.
>> It is better to call skb_dst_drop() from error code path.
>
> Do we really want to keep a dst from another namespace if
> skb_scrub_packet() is called with xnet=true ?
>
tun_dst is allocated in same namespace. It is required for LWT to work.