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>
In-Reply-To: <20190428030539.17776-1-yuehaibing@huawei.com>
From:   Cong Wang <xiyou.wangcong@gmail.com>
Date:   Sun, 28 Apr 2019 10:49:25 -0700
Message-ID: <CAM_iQpX5RgaixruwWW=3YdMHR69yjcPME+bXnC0PozxOnCM0hg@mail.gmail.com>
Subject: Re: [PATCH] tun: Fix use-after-free in tun_net_xmit
To:     Yue Haibing <yuehaibing@huawei.com>
Cc:     David Miller <davem@davemloft.net>,
        Jason Wang <jasowang@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        "Li,Rongqing" <lirongqing@baidu.com>,
        Nicolas Dichtel <nicolas.dichtel@6wind.com>,
        Chas Williams <3chas3@gmail.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 Sat, Apr 27, 2019 at 8:06 PM Yue Haibing <yuehaibing@huawei.com> wrote:
>
> If tun driver have multiqueues, user close the last queue by
> tun_detach, then tun->tfiles[index] is not cleared. Then a new
> queue may add to the tun, which using rcu_assign_pointer
> tun->tfiles[index] to the new tfile and increase the numqueues.
> However if there send a packet during this time, which picking the last
> queue, it may uses the old tun->tfiles[index], beacause there no
> RCU grace period.

This analysis makes sense. It is a normal scenario for RCU,
where readers could still read even after we unpublish the RCU
protected structure, we only need to worry about when we free it.


> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e9ca1c0..3770aba 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -876,6 +876,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>          */
>         rcu_assign_pointer(tfile->tun, tun);
>         rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
> +       synchronize_net();
>         tun->numqueues++;
>         tun_set_real_num_queues(tun);

But this fix doesn't make any sense, we only wait for RCU
grace period when freeing old ones, not for new ones. RCU
grace period is all about readers against free.

This is why I came up with the SOCK_RCU_FREE patch, which
is also blocking-free.


Thanks.