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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <00c1f8e162f2b5b50d0326738230a7a6f55d971e.camel@oracle.com>
Date: Mon, 14 Jul 2025 05:06:22 +0000
From: Allison Henderson <allison.henderson@...cle.com>
To: "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC][PATCH] don't open-code kernel_accept() in
 rds_tcp_accept_one()

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