Date: Wed, 29 Jan 2014 15:48:11 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: nicolas.dichtel@6wind.com
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        stable <stable@vger.kernel.org>, Clark Williams <williams@redhat.com>,
        "Luis Claudio R. Goncalves" <lclaudio@uudg.org>,
        John Kacur <jkacur@redhat.com>, Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH 0/2] [BUG FIXES - 3.10.27] sit: More backports
Message-ID: <20140129154811.27fbaf13@gandalf.local.home>
In-Reply-To: <52E9267C.90403@6wind.com>
References: <20140128205756.074448668@goodmis.org>
	<52E8E025.2060803@6wind.com>
	<20140129075745.4227a2ed@gandalf.local.home>
	<52E9267C.90403@6wind.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

On Wed, 29 Jan 2014 17:04:12 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
 
> Your patch serie seems to be the good way to go (note that patch 1/2 does not
> compile) but I think the fix is smaller because we don't have x-netns.
> 
> Here is my proposal, if you agree, I will send the same patch for ip6_tunnnel,
> which have the netns leak.
> 

Hold on. Seems that the kernels that were being tested in QA had more
code than what I was testing. Clark had backported "sit: fix use after
free of fb_tunnel_dev" and that was what was causing the
unlist_netdevice() to be missed.

When I started working on vanilla 3.10.27 as well, I first did my
original patch (which just removes the call to
unregister_netdevice_queue() from sit_exit_net()). I asked to have that
added to our kernel for testing, and they told me it was already there
via Clark's backport. Then I did the full backport as well and looked
for the leak. I'm now thinking that the full backport is not needed as
that was what caused the leak.

According to commit 9434266f2c645d4fcf62a03a8e36ad8075e37943 "sit: fix
use after free of fb_tunnel_dev", it states:

    Bug: The fallback device is created in sit_init_net and assumed to
    be freed in sit_exit_net. First, it is dereferenced in that
    function, in sit_destroy_tunnels:
    
            struct net *net = dev_net(sitn->fb_tunnel_dev);
    
    Prior to this, rtnl_unlink_register has removed all devices that
    match rtnl_link_ops == sit_link_ops.

    Commit 205983c43700 added the line
    
    +       sitn->fb_tunnel_dev->rtnl_link_ops = &sit_link_ops;
    
    which cases the fallback device to match here and be freed before it
    is last dereferenced.

Commit 205983c43700 was backported to 3.10, but without commit
5e6700b3bf98 "sit: add support of x-netns" which was what added the

  net = dev_net(sitn->fb_tunnel_dev);

Which looks to me that the only reason I need to port back commit
5e6700b3bf98 is if I add the full backport of 9434266f2c645d4f.

Seems to me that my original patch may be good enough. The one that I
said this series obsoletes.

Note, I've talked with the people that are doing the testing, and I'm
having them revert all changes except for that one fix and rerun the
tests again. I should know the results by tomorrow.

Let me know if "sit: fix use after free of fb_tunnel_dev" still needs
to be backported due to some other way that the fallback device can be
dereferenced after being freed.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/