Re: [ 31/37] tun: dont zeroize sock->file on detach

[Ben Hutchings]

On Fri, 2012-08-17 at 04:03 +0100, Ben Hutchings wrote:
> 3.2-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Stanislav Kinsbursky <skinsbursky@xxxxxxxxxxxxx>
> 
> commit 66d1b9263a371abd15806c53f486f0645ef31a8f upstream.
> 
> This is a fix for bug, introduced in 3.4 kernel by commit
> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d ("tun: don't hold network
> namespace by tun sockets"), which, among other things, replaced simple
> sock_put() by sk_release_kernel(). Below is sequence, which leads to
> oops for non-persistent devices:

I didn't read this message properly when importing cc'd commits.  I'm
going to drop this patch, as it appears that it will result in an inode
leak or other badness if applied to 3.2.y.  Let David Miller know if any
tun fixes should go into 3.2.y.

Ben.

> tun_chr_close()
> tun_detach()				<== tun->socket.file = NULL
> tun_free_netdev()
> sk_release_sock()
> sock_release(sock->file == NULL)
> iput(SOCK_INODE(sock))			<== dereference on NULL pointer
> 
> This patch just removes zeroing of socket's file from __tun_detach().
> sock_release() will do this.
> 
> Reported-by: Ruan Zhijie <ruanzhijie@xxxxxxxxxxx>
> Tested-by: Ruan Zhijie <ruanzhijie@xxxxxxxxxxx>
> Acked-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Acked-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Acked-by: Yuchung Cheng <ycheng@xxxxxxxxxx>
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@xxxxxxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
>  drivers/net/tun.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 926d4db..3a16d4f 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -187,7 +187,6 @@ static void __tun_detach(struct tun_struct *tun)
>  	netif_tx_lock_bh(tun->dev);
>  	netif_carrier_off(tun->dev);
>  	tun->tfile = NULL;
> -	tun->socket.file = NULL;
>  	netif_tx_unlock_bh(tun->dev);
>  
>  	/* Drop read queue */
> 

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

Attachment: signature.asc
Description: This is a digitally signed message part