| Message ID | 168321389545.16695.14828237648251844351.stgit@oracle-102.nfsv4bat.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bug fixes for net/handshake | expand |
On Thu, May 04, 2023 at 11:25:05AM -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> Reviewed-by: Simon Horman <simon.horman@corigine.com>
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;
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
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); 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 */ return 0; out_complete: handshake_complete(req, -EIO, NULL); - fput(sock->file); out_status: trace_handshake_cmd_accept_err(net, req, NULL, err); return err;