Re: [PATCH v1] SUNRPC: Reject a socket that already has an svc_sock attached

On Sat, 2026-08-15 at 12:28 -0400, Chuck Lever wrote:
> Writing the same socket descriptor to /proc/fs/nfsd/portlist twice
> attaches a second svc_sock to one socket. svc_setup_socket() saves
> the socket's callbacks before installing its own, so the second
> attach records svc_write_space() as the old write_space callback.
> svc_udp_init() invokes that callback by way of svc_sock_setbufsize(),
> and svc_write_space() then calls itself until the kernel stack is
> exhausted:
> 
>   BUG: TASK stack guard page was hit at ffffc900037d7ff8
>    svc_write_space+0x90/0x2b0 net/sunrpc/svcsock.c:429
>    svc_write_space+0xe6/0x2b0 net/sunrpc/svcsock.c:430
>    ... 700 more ...
>    svc_sock_setbufsize+0x18d/0x220 net/sunrpc/svcsock.c:386
>    svc_udp_init net/sunrpc/svcsock.c:854 [inline]
>    svc_setup_socket+0xb2f/0x1090 net/sunrpc/svcsock.c:1498
>    svc_addsock+0x2fd/0x760 net/sunrpc/svcsock.c:1547
>    __write_ports_addfd fs/nfsd/nfsctl.c:742 [inline]
>    write_ports+0xa5b/0xcc0 fs/nfsd/nfsctl.c:861
>    nfsctl_transaction_write+0x106/0x1a0 fs/nfsd/nfsctl.c:112
> 
> svc_data_ready() and svc_tcp_state_change() chain through their saved
> callbacks the same way, so a TCP descriptor added twice recurses on
> the next incoming segment instead. Reaching any of this takes a
> writer on portlist, and the nfsd filesystem sets no FS_USERNS_MOUNT,
> so the reproducer needs CAP_SYS_ADMIN in the initial user namespace.
> 
> Reject a socket that already carries sk_user_data. svc_setup_socket()
> overwrites that field unconditionally, so a socket some other
> consumer has claimed is one NFSD would corrupt whether or not the
> callbacks recurse.
> 
> Fixes: b41b66d63c73 ("[PATCH] knfsd: allow sockets to be passed to nfsd via 'portlist'")
> Reported-by: syzbot+54cdc566f64abf51b7f1-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org
> Closes: https://syzkaller.appspot.com/bug?extid=54cdc566f64abf51b7f1
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>  net/sunrpc/svcsock.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 7a423e9ee74d..5a2d52284d75 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1614,6 +1614,9 @@ int svc_addsock(struct svc_serv *serv, struct net *net, const int fd,
>  	err = -EISCONN;
>  	if (so->state > SS_UNCONNECTED)
>  		goto out;
> +	err = -EBUSY;
> +	if (so->sk->sk_user_data)
> +		goto out;
>  	err = -ENOENT;
>  	if (!try_module_get(THIS_MODULE))
>  		goto out;

Nice catch!

Reviewed-by: Jeff Layton <[email protected]>

Claude had some pedantry about the changelog though. Regurgitated
verbatim here:

> svc_data_ready() and svc_tcp_state_change() chain through their saved
> callbacks the same way, so a TCP descriptor added twice recurses on
> the next incoming segment instead.

Are those the two callbacks that recurse for tcp?

The write_ports kerneldoc says "listen(3) must be called for a
SOCK_STREAM socket", and for a listening socket svc_tcp_init() only
replaces sk_data_ready:

net/sunrpc/svcsock.c:svc_tcp_init() {
	...
	if (sk->sk_state == TCP_LISTEN) {
		strcpy(svsk->sk_xprt.xpt_remotebuf, "listener");
		set_bit(XPT_LISTENER, &svsk->sk_xprt.xpt_flags);
		set_bit(XPT_RPCB_UNREG, &svsk->sk_xprt.xpt_flags);
		sk->sk_data_ready = svc_tcp_listen_data_ready;
		set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
	} else {
		sk->sk_state_change = svc_tcp_state_change;
		sk->sk_data_ready = svc_data_ready;
		sk->sk_write_space = svc_write_space;
	...
}

sk_state_change and sk_write_space are left alone on a listener, so the
second svc_setup_socket() saves svc_tcp_listen_data_ready() into
sk_odata and the recursion runs there instead:

net/sunrpc/svcsock.c:svc_tcp_listen_data_ready() {
	struct svc_sock	*svsk = (struct svc_sock *)sk-
>sk_user_data;
	...
	if (svsk) {
		/* Refer to svc_setup_socket() for details. */
		rmb();
		svsk->sk_odata(sk);
	...
}

The else branch does install svc_data_ready() and
svc_tcp_state_change(), but svc_addsock() only reaches it for a
descriptor that is not listening, and svc_tcp_init() then closes that
transport right away:

net/sunrpc/svcsock.c:svc_tcp_init() {
	...
		switch (sk->sk_state) {
		case TCP_SYN_RECV:
		case TCP_ESTABLISHED:
			break;
		default:
			svc_xprt_deferred_close(&svsk->sk_xprt);
		}
	...
}

Should the paragraph name svc_tcp_listen_data_ready() instead?