[PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[Moritz Wanzenböck]

If req->hr_proto->hp_accept() fail, we call fput() twice:
Once in the error path, but also a second time because sock->file
is at that point already associated with the file descriptor. Once
the task exits, as it would probably do after receiving an error
reading from netlink, the fd is closed, calling fput() a second time.

To fix, we move installing the file after the error path for the
hp_accept() call. In the case of errors we simply put the unused fd.
In case of success we can use fd_install() to link the sock->file
to the reserved fd.

Fixes: 7ea9c1ec66bc ("net/handshake: Fix handshake_dup() ref counting")
Signed-off-by: Moritz Wanzenböck <moritz.wanzenboeck@linbit.com>
---
 net/handshake/netlink.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 64a0046dd611..89637e732866 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -87,29 +87,6 @@ struct nlmsghdr *handshake_genl_put(struct sk_buff *msg,
 }
 EXPORT_SYMBOL(handshake_genl_put);
 
-/*
- * dup() a kernel socket for use as a user space file descriptor
- * in the current process. The kernel socket must have an
- * instatiated struct file.
- *
- * Implicit argument: "current()"
- */
-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);
-		return newfd;
-	}
-
-	fd_install(newfd, file);
-	return newfd;
-}
-
 int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
@@ -133,17 +110,20 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 		goto out_status;
 
 	sock = req->hr_sk->sk_socket;
-	fd = handshake_dup(sock);
+	fd = get_unused_fd_flags(O_CLOEXEC);
 	if (fd < 0) {
 		err = fd;
 		goto out_complete;
 	}
+
 	err = req->hr_proto->hp_accept(req, info, fd);
 	if (err) {
-		fput(sock->file);
+		put_unused_fd(fd);
 		goto out_complete;
 	}
 
+	fd_install(fd, get_file(sock->file));
+
 	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
 	return 0;
 
-- 
2.41.0

Re: [PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[Hannes Reinecke]

On 10/19/23 14:58, Moritz Wanzenböck wrote:
> If req->hr_proto->hp_accept() fail, we call fput() twice:
> Once in the error path, but also a second time because sock->file
> is at that point already associated with the file descriptor. Once
> the task exits, as it would probably do after receiving an error
> reading from netlink, the fd is closed, calling fput() a second time.
> 
> To fix, we move installing the file after the error path for the
> hp_accept() call. In the case of errors we simply put the unused fd.
> In case of success we can use fd_install() to link the sock->file
> to the reserved fd.
> 
> Fixes: 7ea9c1ec66bc ("net/handshake: Fix handshake_dup() ref counting")
> Signed-off-by: Moritz Wanzenböck <moritz.wanzenboeck@linbit.com>
> ---
>   net/handshake/netlink.c | 30 +++++-------------------------
>   1 file changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 64a0046dd611..89637e732866 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -87,29 +87,6 @@ struct nlmsghdr *handshake_genl_put(struct sk_buff *msg,
>   }
>   EXPORT_SYMBOL(handshake_genl_put);
>   
> -/*
> - * dup() a kernel socket for use as a user space file descriptor
> - * in the current process. The kernel socket must have an
> - * instatiated struct file.
> - *
> - * Implicit argument: "current()"
> - */
> -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);
> -		return newfd;
> -	}
> -
> -	fd_install(newfd, file);
> -	return newfd;
> -}
> -
>   int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>   {
>   	struct net *net = sock_net(skb->sk);
> @@ -133,17 +110,20 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>   		goto out_status;
>   
>   	sock = req->hr_sk->sk_socket;
> -	fd = handshake_dup(sock);
> +	fd = get_unused_fd_flags(O_CLOEXEC);
>   	if (fd < 0) {
>   		err = fd;
>   		goto out_complete;
>   	}
> +
>   	err = req->hr_proto->hp_accept(req, info, fd);
>   	if (err) {
> -		fput(sock->file);
> +		put_unused_fd(fd);
>   		goto out_complete;
>   	}
>   
> +	fd_install(fd, get_file(sock->file));
> +
>   	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
>   	return 0;
>   
??
You sure this works?
With this patch 'fd' won't be available during the 'hp_accept' call,
but I thought that we might want to read from the socket here.
Hmm?

Cheers,

Hannes

Re: [PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[Moritz Wanzenböck]

On Thu, Oct 19, 2023 at 3:06 PM Hannes Reinecke <hare@suse.de> wrote:
>
> ??
> You sure this works?
> With this patch 'fd' won't be available during the 'hp_accept' call,
> but I thought that we might want to read from the socket here.
> Hmm?

Yes, this works. 'hp_accept' only constructs the netlink message for
user space, so it only needs 'fd' for its integer value.

Regards,
Moritz

Re: [PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[Chuck Lever]

On Thu, Oct 19, 2023 at 02:58:47PM +0200, Moritz Wanzenböck wrote:
> If req->hr_proto->hp_accept() fail, we call fput() twice:
> Once in the error path, but also a second time because sock->file
> is at that point already associated with the file descriptor. Once
> the task exits, as it would probably do after receiving an error
> reading from netlink, the fd is closed, calling fput() a second time.
> 
> To fix, we move installing the file after the error path for the
> hp_accept() call. In the case of errors we simply put the unused fd.
> In case of success we can use fd_install() to link the sock->file
> to the reserved fd.
> 
> Fixes: 7ea9c1ec66bc ("net/handshake: Fix handshake_dup() ref counting")
> Signed-off-by: Moritz Wanzenböck <moritz.wanzenboeck@linbit.com>
> ---
>  net/handshake/netlink.c | 30 +++++-------------------------
>  1 file changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 64a0046dd611..89637e732866 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -87,29 +87,6 @@ struct nlmsghdr *handshake_genl_put(struct sk_buff *msg,
>  }
>  EXPORT_SYMBOL(handshake_genl_put);
>  
> -/*
> - * dup() a kernel socket for use as a user space file descriptor
> - * in the current process. The kernel socket must have an
> - * instatiated struct file.
> - *
> - * Implicit argument: "current()"
> - */
> -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);
> -		return newfd;
> -	}
> -
> -	fd_install(newfd, file);
> -	return newfd;
> -}
> -

The point of the handshake_dup() helper was to underscore the
similarities between dup(2) and handshake ACCEPT. That similarity
gets fainter over time.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


>  int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct net *net = sock_net(skb->sk);
> @@ -133,17 +110,20 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto out_status;
>  
>  	sock = req->hr_sk->sk_socket;
> -	fd = handshake_dup(sock);
> +	fd = get_unused_fd_flags(O_CLOEXEC);
>  	if (fd < 0) {
>  		err = fd;
>  		goto out_complete;
>  	}
> +
>  	err = req->hr_proto->hp_accept(req, info, fd);
>  	if (err) {
> -		fput(sock->file);
> +		put_unused_fd(fd);
>  		goto out_complete;
>  	}
>  
> +	fd_install(fd, get_file(sock->file));
> +
>  	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
>  	return 0;
>  
> -- 
> 2.41.0
> 
> 

-- 
Chuck Lever

Re: [PATCH] net/handshake: fix file ref count in handshake_nl_accept_doit()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 19 Oct 2023 14:58:47 +0200 you wrote:
> If req->hr_proto->hp_accept() fail, we call fput() twice:
> Once in the error path, but also a second time because sock->file
> is at that point already associated with the file descriptor. Once
> the task exits, as it would probably do after receiving an error
> reading from netlink, the fd is closed, calling fput() a second time.
> 
> To fix, we move installing the file after the error path for the
> hp_accept() call. In the case of errors we simply put the unused fd.
> In case of success we can use fd_install() to link the sock->file
> to the reserved fd.
> 
> [...]

Here is the summary with links:
  - net/handshake: fix file ref count in handshake_nl_accept_doit()
    https://git.kernel.org/netdev/net/c/7798b59409c3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html