[PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Steffen Klassert]

We might extend the used aera of a skb beyond the total
headroom when we install the ipip header. Fix this by
calling skb_cow_head() unconditionally.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/ip_tunnel.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index ac9fabe..b8ce640 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -641,13 +641,13 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	max_headroom = LL_RESERVED_SPACE(rt->dst.dev) + sizeof(struct iphdr)
 			+ rt->dst.header_len;
-	if (max_headroom > dev->needed_headroom) {
+	if (max_headroom > dev->needed_headroom)
 		dev->needed_headroom = max_headroom;
-		if (skb_cow_head(skb, dev->needed_headroom)) {
-			dev->stats.tx_dropped++;
-			dev_kfree_skb(skb);
-			return;
-		}
+
+	if (skb_cow_head(skb, dev->needed_headroom)) {
+		dev->stats.tx_dropped++;
+		dev_kfree_skb(skb);
+		return;
 	}
 
 	err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, protocol,
-- 
1.7.9.5

[PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Steffen Klassert]

Currently we can not update the tunnel parameters of
the fallback tunnels because we don't find them in the
hash lists. Fix this by adding them on initialization.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/ip_tunnel.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index b8ce640..f2348f2 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 	/* FB netdevice is special: we have one, and only one per netns.
 	 * Allowing to move it to another netns is clearly unsafe.
 	 */
-	if (!IS_ERR(itn->fb_tunnel_dev))
+	if (!IS_ERR(itn->fb_tunnel_dev)) {
 		itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+		ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));
+	}
 	rtnl_unlock();
 
 	return PTR_RET(itn->fb_tunnel_dev);
-- 
1.7.9.5

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Eric Dumazet]

On Wed, 2013-09-25 at 07:54 +0200, Steffen Klassert wrote:
> We might extend the used aera of a skb beyond the total
> headroom when we install the ipip header. Fix this by
> calling skb_cow_head() unconditionally.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/ipv4/ip_tunnel.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Do you know or can we find when was the bug introduced ?

(commit id and title in your changelog would be really nice)

Thanks !

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Steffen Klassert]

On Wed, Sep 25, 2013 at 04:56:03AM -0700, Eric Dumazet wrote:
> On Wed, 2013-09-25 at 07:54 +0200, Steffen Klassert wrote:
> > We might extend the used aera of a skb beyond the total
> > headroom when we install the ipip header. Fix this by
> > calling skb_cow_head() unconditionally.
> > 
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > ---
> >  net/ipv4/ip_tunnel.c |   12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Do you know or can we find when was the bug introduced ?

It was commit c544193214 (GRE: Refactor GRE tunneling code.)
when net/ipv4/ip_tunnel.c was created.

> 
> (commit id and title in your changelog would be really nice)
> 

I can send a v2 with these informations included if you want
that.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Eric Dumazet]

On Wed, 2013-09-25 at 14:13 +0200, Steffen Klassert wrote:
> It was commit c544193214 (GRE: Refactor GRE tunneling code.)
> when net/ipv4/ip_tunnel.c was created.

Yeah. This one begins to be very upsetting.

> 
> > 
> > (commit id and title in your changelog would be really nice)
> > 
> 
> I can send a v2 with these informations included if you want
> that.

I think that patches coming from experimented kernel developers
should always include a study of bug origin.

Otherwise, both maintainer and stable teams have to figure it, and
they sometime lack the time or context.

Plus it's good to CC patch author and reviewers so that he can learn of
its mistakes, and Ack your work as well.

Thanks

Re: [PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Pravin Shelar]

On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> Currently we can not update the tunnel parameters of
> the fallback tunnels because we don't find them in the
> hash lists. Fix this by adding them on initialization.
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/ipv4/ip_tunnel.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index b8ce640..f2348f2 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
>         /* FB netdevice is special: we have one, and only one per netns.
>          * Allowing to move it to another netns is clearly unsafe.
>          */
> -       if (!IS_ERR(itn->fb_tunnel_dev))
> +       if (!IS_ERR(itn->fb_tunnel_dev)) {
>                 itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
> +               ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));
> +       }
>         rtnl_unlock();
>
fallback tunnel s not required to be in hash table, Its is returned if
none of hashed tunnels are matched, ref ip_tunnel_lookup().
Can you post command to reproduce this issue?

>         return PTR_RET(itn->fb_tunnel_dev);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Pravin Shelar]

On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> We might extend the used aera of a skb beyond the total
> headroom when we install the ipip header. Fix this by
> calling skb_cow_head() unconditionally.
>
It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
consistent with gre.

> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/ipv4/ip_tunnel.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index ac9fabe..b8ce640 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -641,13 +641,13 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>
>         max_headroom = LL_RESERVED_SPACE(rt->dst.dev) + sizeof(struct iphdr)
>                         + rt->dst.header_len;
> -       if (max_headroom > dev->needed_headroom) {
> +       if (max_headroom > dev->needed_headroom)
>                 dev->needed_headroom = max_headroom;
> -               if (skb_cow_head(skb, dev->needed_headroom)) {
> -                       dev->stats.tx_dropped++;
> -                       dev_kfree_skb(skb);
> -                       return;
> -               }
> +
> +       if (skb_cow_head(skb, dev->needed_headroom)) {
> +               dev->stats.tx_dropped++;
> +               dev_kfree_skb(skb);
> +               return;
>         }
>
>         err = iptunnel_xmit(rt, skb, fl4.saddr, fl4.daddr, protocol,
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Steffen Klassert]

On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote:
> On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > Currently we can not update the tunnel parameters of
> > the fallback tunnels because we don't find them in the
> > hash lists. Fix this by adding them on initialization.
> >
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > ---
> >  net/ipv4/ip_tunnel.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> > index b8ce640..f2348f2 100644
> > --- a/net/ipv4/ip_tunnel.c
> > +++ b/net/ipv4/ip_tunnel.c
> > @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
> >         /* FB netdevice is special: we have one, and only one per netns.
> >          * Allowing to move it to another netns is clearly unsafe.
> >          */
> > -       if (!IS_ERR(itn->fb_tunnel_dev))
> > +       if (!IS_ERR(itn->fb_tunnel_dev)) {
> >                 itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
> > +               ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));
> > +       }
> >         rtnl_unlock();
> >
> fallback tunnel s not required to be in hash table, Its is returned if
> none of hashed tunnels are matched, ref ip_tunnel_lookup().
> Can you post command to reproduce this issue?
> 

Something like

ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1

worked until v3.9 and stopped working with v3.10.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Steffen Klassert]

On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:
> On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > We might extend the used aera of a skb beyond the total
> > headroom when we install the ipip header. Fix this by
> > calling skb_cow_head() unconditionally.
> >
> It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
> consistent with gre.

I think this would just move the bug from ipip to gre. ipgre_xmit()
uses dev->needed_headroom which is based on the guessed output device
in ip_tunnel_bind_dev(). If the device we get from the route lookup
in ip_tunnel_xmit() is different from the guessed one and the resulting
max_headroom is bigger than dev->needed_headroom, we run into that bug
because skb_cow_head() will not be called with the updated
dev->needed_headroom.

Re: [PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Pravin Shelar]

On Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote:
>> On Tue, Sep 24, 2013 at 10:55 PM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>> > Currently we can not update the tunnel parameters of
>> > the fallback tunnels because we don't find them in the
>> > hash lists. Fix this by adding them on initialization.
>> >
>> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>> > ---
>> >  net/ipv4/ip_tunnel.c |    4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> > index b8ce640..f2348f2 100644
>> > --- a/net/ipv4/ip_tunnel.c
>> > +++ b/net/ipv4/ip_tunnel.c
>> > @@ -853,8 +853,10 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
>> >         /* FB netdevice is special: we have one, and only one per netns.
>> >          * Allowing to move it to another netns is clearly unsafe.
>> >          */
>> > -       if (!IS_ERR(itn->fb_tunnel_dev))
>> > +       if (!IS_ERR(itn->fb_tunnel_dev)) {
>> >                 itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
>> > +               ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));
>> > +       }
>> >         rtnl_unlock();
>> >
>> fallback tunnel s not required to be in hash table, Its is returned if
>> none of hashed tunnels are matched, ref ip_tunnel_lookup().
>> Can you post command to reproduce this issue?
>>
>
> Something like
>
> ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1
>
> worked until v3.9 and stopped working with v3.10.

OK, I see the bug, tunnel exact match lookup does not check fb tunnel.
There are two options.
1. Fix ip_tunnel_find() to check for fb tunnel.
2. Add fb tunnel to hash table, which is what ur patch does.
I think your patch is better solution as it get rid of special case.
But patch is not complete. It needs to remove fb tunnel checks on
netdev unregister.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Pravin Shelar]

On Thu, Sep 26, 2013 at 1:25 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:
>> On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>> > We might extend the used aera of a skb beyond the total
>> > headroom when we install the ipip header. Fix this by
>> > calling skb_cow_head() unconditionally.
>> >
>> It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
>> consistent with gre.
>
> I think this would just move the bug from ipip to gre. ipgre_xmit()
> uses dev->needed_headroom which is based on the guessed output device
> in ip_tunnel_bind_dev(). If the device we get from the route lookup
> in ip_tunnel_xmit() is different from the guessed one and the resulting
> max_headroom is bigger than dev->needed_headroom, we run into that bug
> because skb_cow_head() will not be called with the updated
> dev->needed_headroom.
>
Thats why ip_tunnel_xmit() update dev->needed_headroom.
Just to be clear I was talking abt calling skb_cow_head with
dev->needed_headroom in ipip_xmit and leave ip_tunnel_xmit as it is.
So that most of cases we will not need to adjust headroom in ip_tunnel
xmit.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Steffen Klassert]

On Thu, Sep 26, 2013 at 11:25:01AM -0700, Pravin Shelar wrote:
> On Thu, Sep 26, 2013 at 1:25 AM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:
> >> On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
> >> <steffen.klassert@secunet.com> wrote:
> >> > We might extend the used aera of a skb beyond the total
> >> > headroom when we install the ipip header. Fix this by
> >> > calling skb_cow_head() unconditionally.
> >> >
> >> It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
> >> consistent with gre.
> >
> > I think this would just move the bug from ipip to gre. ipgre_xmit()
> > uses dev->needed_headroom which is based on the guessed output device
> > in ip_tunnel_bind_dev(). If the device we get from the route lookup
> > in ip_tunnel_xmit() is different from the guessed one and the resulting
> > max_headroom is bigger than dev->needed_headroom, we run into that bug
> > because skb_cow_head() will not be called with the updated
> > dev->needed_headroom.
> >
> Thats why ip_tunnel_xmit() update dev->needed_headroom.
> Just to be clear I was talking abt calling skb_cow_head with
> dev->needed_headroom in ipip_xmit and leave ip_tunnel_xmit as it is.
> So that most of cases we will not need to adjust headroom in ip_tunnel
> xmit.

skb_cow_head() does not do much if there is enough headroom, so
calling it here is uncritical. But we should adjust the headroom
as soon as we know that it is insufficient.

Also, I really wonder how you want to adjust the headroom in
ipip_tunnel_xmit() to a correct value. We know the needed
headroom after the route lookup in ip_tunnel_xmit() and
we have to adust it here because ip_tunnel_xmit() calls
iptunnel_xmit() which does a __skb_push() before it
installs the IP header.

Please keep in mind tat this is a bug fix that might be interesting
for stable too, we should try to keep the changes at a minimum.

Another thing that I noticed, with commit 0e6fbc5b
(ip_tunnels: extend iptunnel_xmit()) you moved the IP header
installation to iptunnel_xmit() and changed skb_push()
to __skb_push(). This made this bug quite hard to track
down because instead of triggering a skb under panic,
it did a silent memory corruption and crashed at random
other places. Maybe we should change this back to skb_push().

Re: [PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Steffen Klassert]

On Thu, Sep 26, 2013 at 11:24:07AM -0700, Pravin Shelar wrote:
> On Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote:
> >> fallback tunnel s not required to be in hash table, Its is returned if
> >> none of hashed tunnels are matched, ref ip_tunnel_lookup().
> >> Can you post command to reproduce this issue?
> >>
> >
> > Something like
> >
> > ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1
> >
> > worked until v3.9 and stopped working with v3.10.
> 
> OK, I see the bug, tunnel exact match lookup does not check fb tunnel.
> There are two options.
> 1. Fix ip_tunnel_find() to check for fb tunnel.
> 2. Add fb tunnel to hash table, which is what ur patch does.
> I think your patch is better solution as it get rid of special case.
> But patch is not complete. It needs to remove fb tunnel checks on
> netdev unregister.

It looks like this is another bug that requires an additional patch.
We add the fallback tunnel to the unregister list when we iterate over
all netdevices in the namespace at the beginning of ip_tunnel_destroy()
and then again explicitly at the end of ip_tunnel_destroy().

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Pravin Shelar]

On Fri, Sep 27, 2013 at 12:45 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Thu, Sep 26, 2013 at 11:25:01AM -0700, Pravin Shelar wrote:
>> On Thu, Sep 26, 2013 at 1:25 AM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>> > On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:
>> >> On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
>> >> <steffen.klassert@secunet.com> wrote:
>> >> > We might extend the used aera of a skb beyond the total
>> >> > headroom when we install the ipip header. Fix this by
>> >> > calling skb_cow_head() unconditionally.
>> >> >
>> >> It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
>> >> consistent with gre.
>> >
>> > I think this would just move the bug from ipip to gre. ipgre_xmit()
>> > uses dev->needed_headroom which is based on the guessed output device
>> > in ip_tunnel_bind_dev(). If the device we get from the route lookup
>> > in ip_tunnel_xmit() is different from the guessed one and the resulting
>> > max_headroom is bigger than dev->needed_headroom, we run into that bug
>> > because skb_cow_head() will not be called with the updated
>> > dev->needed_headroom.
>> >
>> Thats why ip_tunnel_xmit() update dev->needed_headroom.
>> Just to be clear I was talking abt calling skb_cow_head with
>> dev->needed_headroom in ipip_xmit and leave ip_tunnel_xmit as it is.
>> So that most of cases we will not need to adjust headroom in ip_tunnel
>> xmit.
>
> skb_cow_head() does not do much if there is enough headroom, so
> calling it here is uncritical. But we should adjust the headroom
> as soon as we know that it is insufficient.
>
ok.

> Also, I really wonder how you want to adjust the headroom in
> ipip_tunnel_xmit() to a correct value. We know the needed
> headroom after the route lookup in ip_tunnel_xmit() and
> we have to adust it here because ip_tunnel_xmit() calls
> iptunnel_xmit() which does a __skb_push() before it
> installs the IP header.
>
> Please keep in mind tat this is a bug fix that might be interesting
> for stable too, we should try to keep the changes at a minimum.
>
> Another thing that I noticed, with commit 0e6fbc5b
> (ip_tunnels: extend iptunnel_xmit()) you moved the IP header
> installation to iptunnel_xmit() and changed skb_push()
> to __skb_push(). This made this bug quite hard to track
> down because instead of triggering a skb under panic,
> it did a silent memory corruption and crashed at random
> other places. Maybe we should change this back to skb_push().

All callers of iptunnel_xmit() are required to setup sufficient
headroom. So skb_push check are not necessary.

Re: [PATCH net 2/2] ip_tunnel: Add fallback tunnels to the hash lists

[Pravin Shelar]

On Fri, Sep 27, 2013 at 12:56 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Thu, Sep 26, 2013 at 11:24:07AM -0700, Pravin Shelar wrote:
>> On Thu, Sep 26, 2013 at 1:13 AM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>> > On Wed, Sep 25, 2013 at 09:03:11AM -0700, Pravin Shelar wrote:
>> >> fallback tunnel s not required to be in hash table, Its is returned if
>> >> none of hashed tunnels are matched, ref ip_tunnel_lookup().
>> >> Can you post command to reproduce this issue?
>> >>
>> >
>> > Something like
>> >
>> > ip tunnel change tunl0 mode ipip remote 0.0.0.0 local 0.0.0.0 ttl 0 tos 1
>> >
>> > worked until v3.9 and stopped working with v3.10.
>>
>> OK, I see the bug, tunnel exact match lookup does not check fb tunnel.
>> There are two options.
>> 1. Fix ip_tunnel_find() to check for fb tunnel.
>> 2. Add fb tunnel to hash table, which is what ur patch does.
>> I think your patch is better solution as it get rid of special case.
>> But patch is not complete. It needs to remove fb tunnel checks on
>> netdev unregister.
>
> It looks like this is another bug that requires an additional patch.
> We add the fallback tunnel to the unregister list when we iterate over
> all netdevices in the namespace at the beginning of ip_tunnel_destroy()
> and then again explicitly at the end of ip_tunnel_destroy().

right.
And if the special case is removed, code will become simple.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Fri, 27 Sep 2013 19:34:59 -0700

> All callers of iptunnel_xmit() are required to setup sufficient
> headroom. So skb_push check are not necessary.

This bug shows that such a check is needed, and would have saved
people like Steffen lots of time tracking down the problem.

I think we should re-instate the check.

I also think that __skb_push() is quite dangerous, in general.  And if
it is to be used at all, it should only be used in circumstances where
all of the context necessary to assert that it cannot underflow the
buffer are right there in the same function.

In fact, the whole damn reason for the assertions in skb_push() is the
catch cases where preconditions are not met across functional
boundaries.  Exactly like the case here.

So again, __skb_push() should be changed back to skb_push() here.

Steffen can you respin these patches and make sure to:

1) Add reference to SHA1_ID and commit header line of commit
   introducing this bug, as Eric requested, in this format:

	$SHA1_ID ("Commit header line text.")

2) __skb_push() --> skb_push()

Thank you.

Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

[Steffen Klassert]

On Mon, Sep 30, 2013 at 02:40:26PM -0400, David Miller wrote:
> 
> Steffen can you respin these patches and make sure to:
> 
> 1) Add reference to SHA1_ID and commit header line of commit
>    introducing this bug, as Eric requested, in this format:
> 
> 	$SHA1_ID ("Commit header line text.")

Sure, will do that.

> 
> 2) __skb_push() --> skb_push()
> 

I'll do an additional patch for this, as it was not introduced
with the same commit as the memory corruption I've fixed here.

I'll also add a patch to fix the double unregister of the fallback
device that was introduced with commit 6c742e714d8
("ipip: add x-netns support").