Re: [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 04 May 2023 11:25:05 -0400 Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> twice.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 7ec8a76c3c8a..3508bc3e661d 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -96,17 +96,13 @@ EXPORT_SYMBOL(handshake_genl_put);
>   */
>  static int handshake_dup(struct socket *sock)
>  {
> -	struct file *file;
>  	int newfd;
>  
> -	file = get_file(sock->file);
>  	newfd = get_unused_fd_flags(O_CLOEXEC);
> -	if (newfd < 0) {
> -		fput(file);
> +	if (newfd < 0)
>  		return newfd;
> -	}
>  
> -	fd_install(newfd, file);
> +	fd_install(newfd, sock->file);

I'm not vfs expert but doesn't this mean that we will now have the file
installed in the fd table, under newfd, before we incremented the
refcount?  Can't another thread close(newfd) and make sock->file
get freed?

>  	return newfd;
>  }
>  
> @@ -143,11 +139,11 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto out_complete;
>  
>  	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
> +	get_file(sock->file);	/* released by DONE */

What if DONE does not get called?

>  	return 0;
>  
>  out_complete:
>  	handshake_complete(req, -EIO, NULL);

We don't want to release the fd here?

> -	fput(sock->file);
>  out_status:
>  	trace_handshake_cmd_accept_err(net, req, NULL, err);
>  	return err;
-- 
pw-bot: cr