netdev - Re: [PATCH net] tuntap: correctly wake up process during uninit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160518140003-mutt-send-email-mst@redhat.com>
Date:	Wed, 18 May 2016 15:41:06 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
	Xi Wang <xii@...gle.com>
Subject: Re: [PATCH net] tuntap: correctly wake up process during uninit

On Wed, May 18, 2016 at 06:58:17PM +0800, Jason Wang wrote:
> We used to check dev->reg_state against NETREG_REGISTERED after each
> time we are woke up. But after commit 9e641bdcfa4e ("net-tun:
> restructure tun_do_read for better sleep/wakeup efficiency"), it uses
> skb_recv_datagram() which does not check dev->reg_state. This will
> result if we delete a tun/tap device after a process is blocked in the
> reading. The device will wait for the reference count which was held
> by that process for ever.
> 
> Fixes this by using RCV_SHUTDOWN which will be checked during
> sk_recv_datagram() before trying to wake up the process during uninit.
> 
> Fixes: 9e641bdcfa4e ("net-tun: restructure tun_do_read for better
> sleep/wakeup efficiency")
> 
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Xi Wang <xii@...gle.com>
> Cc: Michael S. Tsirkin <mst@...hat.com>
> Signed-off-by: Jason Wang <jasowang@...hat.com>

Acked-by: Michael S. Tsirkin <mst@...hat.com>

> ---
> The patch is needed for -stable.
> ---
>  drivers/net/tun.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 425e983..752d849 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -580,11 +580,13 @@ static void tun_detach_all(struct net_device *dev)
>  	for (i = 0; i < n; i++) {
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		BUG_ON(!tfile);
> +		tfile->socket.sk->sk_shutdown = RCV_SHUTDOWN;
>  		tfile->socket.sk->sk_data_ready(tfile->socket.sk);
>  		RCU_INIT_POINTER(tfile->tun, NULL);
>  		--tun->numqueues;
>  	}
>  	list_for_each_entry(tfile, &tun->disabled, next) {
> +		tfile->socket.sk->sk_shutdown = RCV_SHUTDOWN;
>  		tfile->socket.sk->sk_data_ready(tfile->socket.sk);
>  		RCU_INIT_POINTER(tfile->tun, NULL);
>  	}
> @@ -641,6 +643,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file, bool skip_filte
>  			goto out;
>  	}
>  	tfile->queue_index = tun->numqueues;
> +	tfile->socket.sk->sk_shutdown &= ~RCV_SHUTDOWN;
>  	rcu_assign_pointer(tfile->tun, tun);
>  	rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
>  	tun->numqueues++;

By the way I wonder: at the moment interface goes down
each time userspace disconnects, even if it was persistent
and brought up manually (as opposed to on file open).
Should we maybe track manual link up status and keep
persistent device up on userspace disconnect?


> -- 
> 2.7.4