[RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[Al Viro]

	rds_tcp_accept_one() starts with a pretty much verbatim
copy of kernel_accept().  Might as well use the real thing...

	That code went into mainline in 2009, kernel_accept()
had been added in Aug 2006, the copyright on rds/tcp_listen.c
is "Copyright (c) 2006 Oracle", so it's entirely possible
that it predates the introduction of kernel_accept().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index d89bd8d0c354..af36f5bf8649 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -105,10 +105,6 @@ int rds_tcp_accept_one(struct socket *sock)
 	int conn_state;
 	struct rds_conn_path *cp;
 	struct in6_addr *my_addr, *peer_addr;
-	struct proto_accept_arg arg = {
-		.flags = O_NONBLOCK,
-		.kern = true,
-	};
 #if !IS_ENABLED(CONFIG_IPV6)
 	struct in6_addr saddr, daddr;
 #endif
@@ -117,25 +113,9 @@ int rds_tcp_accept_one(struct socket *sock)
 	if (!sock) /* module unload or netns delete in progress */
 		return -ENETUNREACH;
 
-	ret = sock_create_lite(sock->sk->sk_family,
-			       sock->sk->sk_type, sock->sk->sk_protocol,
-			       &new_sock);
+	ret = kernel_accept(sock, &new_sock, O_NONBLOCK);
 	if (ret)
-		goto out;
-
-	ret = sock->ops->accept(sock, new_sock, &arg);
-	if (ret < 0)
-		goto out;
-
-	/* sock_create_lite() does not get a hold on the owner module so we
-	 * need to do it here.  Note that sock_release() uses sock->ops to
-	 * determine if it needs to decrement the reference count.  So set
-	 * sock->ops after calling accept() in case that fails.  And there's
-	 * no need to do try_module_get() as the listener should have a hold
-	 * already.
-	 */
-	new_sock->ops = sock->ops;
-	__module_get(new_sock->ops->owner);
+		return ret;
 
 	rds_tcp_keepalive(new_sock);
 	if (!rds_tcp_tune(new_sock)) {

Re: [RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[Allison Henderson]

On Sun, 2025-07-13 at 19:01 +0100, Al Viro wrote:
> 	rds_tcp_accept_one() starts with a pretty much verbatim
> copy of kernel_accept().  Might as well use the real thing...
> 
> 	That code went into mainline in 2009, kernel_accept()
> had been added in Aug 2006, the copyright on rds/tcp_listen.c
> is "Copyright (c) 2006 Oracle", so it's entirely possible
> that it predates the introduction of kernel_accept().
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index d89bd8d0c354..af36f5bf8649 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -105,10 +105,6 @@ int rds_tcp_accept_one(struct socket *sock)
>  	int conn_state;
>  	struct rds_conn_path *cp;
>  	struct in6_addr *my_addr, *peer_addr;
> -	struct proto_accept_arg arg = {
> -		.flags = O_NONBLOCK,
> -		.kern = true,
> -	};
>  #if !IS_ENABLED(CONFIG_IPV6)
>  	struct in6_addr saddr, daddr;
>  #endif
> @@ -117,25 +113,9 @@ int rds_tcp_accept_one(struct socket *sock)
>  	if (!sock) /* module unload or netns delete in progress */
>  		return -ENETUNREACH;
>  
> -	ret = sock_create_lite(sock->sk->sk_family,
> -			       sock->sk->sk_type, sock->sk->sk_protocol,
> -			       &new_sock);
> +	ret = kernel_accept(sock, &new_sock, O_NONBLOCK);
>  	if (ret)
> -		goto out;
> -
> -	ret = sock->ops->accept(sock, new_sock, &arg);
> -	if (ret < 0)
> -		goto out;
> -
> -	/* sock_create_lite() does not get a hold on the owner module so we
> -	 * need to do it here.  Note that sock_release() uses sock->ops to
> -	 * determine if it needs to decrement the reference count.  So set
> -	 * sock->ops after calling accept() in case that fails.  And there's
> -	 * no need to do try_module_get() as the listener should have a hold
> -	 * already.
> -	 */
> -	new_sock->ops = sock->ops;
> -	__module_get(new_sock->ops->owner);
> +		return ret;
I think we need the "goto out" here, or we will miss the mutex unlock.  Otherwise kernel_accept looks like a pretty
synonymous wrapper.

Thanks!
Allison

>  
>  	rds_tcp_keepalive(new_sock);
>  	if (!rds_tcp_tune(new_sock)) {

Re: [RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[Al Viro]

On Mon, Jul 14, 2025 at 04:36:32AM +0000, Allison Henderson wrote:

> >  	if (!sock) /* module unload or netns delete in progress */
> >  		return -ENETUNREACH;
> >  
> > -	ret = sock_create_lite(sock->sk->sk_family,
> > -			       sock->sk->sk_type, sock->sk->sk_protocol,
> > -			       &new_sock);
> > +	ret = kernel_accept(sock, &new_sock, O_NONBLOCK);
> >  	if (ret)
> > -		goto out;
> > -
> > -	ret = sock->ops->accept(sock, new_sock, &arg);
> > -	if (ret < 0)
> > -		goto out;
> > -
> > -	/* sock_create_lite() does not get a hold on the owner module so we
> > -	 * need to do it here.  Note that sock_release() uses sock->ops to
> > -	 * determine if it needs to decrement the reference count.  So set
> > -	 * sock->ops after calling accept() in case that fails.  And there's
> > -	 * no need to do try_module_get() as the listener should have a hold
> > -	 * already.
> > -	 */
> > -	new_sock->ops = sock->ops;
> > -	__module_get(new_sock->ops->owner);
> > +		return ret;
> I think we need the "goto out" here, or we will miss the mutex unlock.  Otherwise kernel_accept looks like a pretty
> synonymous wrapper.

What mutex_unlock()?
	if (rs_tcp)
		mutex_unlock(&rs_tcp->t_conn_path_lock);
won't be triggered, since rs_tcp remains NULL until
	rs_tcp = rds_tcp_accept_one_path(conn);
well after any of the affected code...

No, return is perfectly fine here - failing kernel_accept() has no side
effects and we have
	if (!sock) /* module unload or netns delete in progress */
		return -ENETUNREACH;
just prior to it.  So if we needed to unlock anything on kernel_accept()
failure, the same would apply for the failure exit just before it...

Re: [RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[Allison Henderson]

On Mon, 2025-07-14 at 05:47 +0100, Al Viro wrote:
> On Mon, Jul 14, 2025 at 04:36:32AM +0000, Allison Henderson wrote:
> 
> > >  	if (!sock) /* module unload or netns delete in progress */
> > >  		return -ENETUNREACH;
> > >  
> > > -	ret = sock_create_lite(sock->sk->sk_family,
> > > -			       sock->sk->sk_type, sock->sk->sk_protocol,
> > > -			       &new_sock);
> > > +	ret = kernel_accept(sock, &new_sock, O_NONBLOCK);
> > >  	if (ret)
> > > -		goto out;
> > > -
> > > -	ret = sock->ops->accept(sock, new_sock, &arg);
> > > -	if (ret < 0)
> > > -		goto out;
> > > -
> > > -	/* sock_create_lite() does not get a hold on the owner module so we
> > > -	 * need to do it here.  Note that sock_release() uses sock->ops to
> > > -	 * determine if it needs to decrement the reference count.  So set
> > > -	 * sock->ops after calling accept() in case that fails.  And there's
> > > -	 * no need to do try_module_get() as the listener should have a hold
> > > -	 * already.
> > > -	 */
> > > -	new_sock->ops = sock->ops;
> > > -	__module_get(new_sock->ops->owner);
> > > +		return ret;
> > I think we need the "goto out" here, or we will miss the mutex unlock.  Otherwise kernel_accept looks like a pretty
> > synonymous wrapper.
> 
> What mutex_unlock()?
> 	if (rs_tcp)
> 		mutex_unlock(&rs_tcp->t_conn_path_lock);
> won't be triggered, since rs_tcp remains NULL until
> 	rs_tcp = rds_tcp_accept_one_path(conn);
> well after any of the affected code...
> 
> No, return is perfectly fine here - failing kernel_accept() has no side
> effects and we have
> 	if (!sock) /* module unload or netns delete in progress */
> 		return -ENETUNREACH;
> just prior to it.  So if we needed to unlock anything on kernel_accept()
> failure, the same would apply for the failure exit just before it...
> 
> 
Oh, you are right, I missed that he assignment came later.  This should do just fine then.  Thanks!
Allison

Re: [RFC][PATCH] don't open-code kernel_accept() in rds_tcp_accept_one()

[patchwork-bot+netdevbpf]

Hello:

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

On Sun, 13 Jul 2025 19:01:34 +0100 you wrote:
> rds_tcp_accept_one() starts with a pretty much verbatim
> copy of kernel_accept().  Might as well use the real thing...
> 
> 	That code went into mainline in 2009, kernel_accept()
> had been added in Aug 2006, the copyright on rds/tcp_listen.c
> is "Copyright (c) 2006 Oracle", so it's entirely possible
> that it predates the introduction of kernel_accept().
> 
> [...]

Here is the summary with links:
  - [RFC] don't open-code kernel_accept() in rds_tcp_accept_one()
    https://git.kernel.org/netdev/net-next/c/5cc7fce3493c

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