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

[Chuck Lever <cel@kernel.org>]

On Fri, May 05, 2023 at 01:58:08PM -0700, Jakub Kicinski wrote:
> 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?

I suppose. I can rework it and send a refresh.


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

A correctly-coded kernel caller has a timeout that is supposed to
release the socket via handshake_req_cancel(). I see that it doesn't
put sock->file, though... perhaps it should use sockfd_put() instead
of sock_release().


> >  	return 0;
> >  
> >  out_complete:
> >  	handshake_complete(req, -EIO, NULL);
> 
> We don't want to release the fd here?

Not any more, since we don't do the get_file() until everything
else has succeeded. But the rework will likely restore the fput
here.


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