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

[Michael S. Tsirkin]

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@xxxxxxxxxx>
> Cc: Xi Wang <xii@xxxxxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>

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

> ---
> 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