Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <71250616-36c1-0d96-8fac-4aaaae6a28d4@redhat.com>
 <20190428030539.17776-1-yuehaibing@huawei.com> <20190429105422-mutt-send-email-mst@kernel.org>
 <CAM_iQpWvp2i6iOZtSPskqU_uXHL2zKfM_cS1rGTh_T0r3BwvnA@mail.gmail.com> <6AADFAC011213A4C87B956458587ADB4021FE16C@dggeml532-mbs.china.huawei.com>
In-Reply-To: <6AADFAC011213A4C87B956458587ADB4021FE16C@dggeml532-mbs.china.huawei.com>
From:   Cong Wang <xiyou.wangcong@gmail.com>
Date:   Tue, 30 Apr 2019 16:42:31 -0700
Message-ID: <CAM_iQpXbQ4X+ohmNFy79D+MPu6remA7hU0DTTypS=Yt+BCxujA@mail.gmail.com>
Subject: Re: [PATCH] tun: Fix use-after-free in tun_net_xmit
To:     "weiyongjun (A)" <weiyongjun1@huawei.com>
Cc:     "Michael S. Tsirkin" <mst@redhat.com>,
        yuehaibing <yuehaibing@huawei.com>,
        David Miller <davem@davemloft.net>,
        Jason Wang <jasowang@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        "Li,Rongqing" <lirongqing@baidu.com>,
        Nicolas Dichtel <nicolas.dichtel@6wind.com>,
        Chas Williams <3chas3@gmail.com>,
        "wangli39@baidu.com" <wangli39@baidu.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 29, 2019 at 10:11 PM weiyongjun (A) <weiyongjun1@huawei.com> wrote:
> This patch should not work. The key point is that when detach the queue
> with index is equal to tun->numqueues - 1, we do not clear the point
> in tun->tfiles:
>
> static void __tun_detach(...)
> {
> ...
>         **** if index == tun->numqueues - 1, nothing changed ****
>         rcu_assign_pointer(tun->tfiles[index],
>                                                 tun->tfiles[tun->numqueues - 1]);
> ....
> }


This is _perfectly_ fine. This is just how we _unpublish_ it, RCU is NOT
against unpublish, you keep missing this point.

Think about list_del_rcu(). RCU readers could still read the list entry
even _after_ list_del_rcu(), this is perfectly fine, list_del_rcu() just
unpublishes the list entry from a global list, kfree_rcu() is the one frees
it. So, RCU readers never hate "unpublish", they just hate "free".


>
> And after tfile free, xmit have change to get and use the freed file point.

With SOCK_RCU_FREE, it won't be freed until the last reader is gone.
This is the fundamental of RCU.

Please, at least look into sk_destruct().

Thanks.