This fixes the refcount overflow, but introduces a socket leak when rpc.tlsservd is actually running: sys_rpctls_syscall() takes an extra ref on the socket. We can't simply stop doing that, as the client-side code in rpctls_connect() expects it. I think the cleanest thing to do is take an explicit ref in rpctls_connect() too.

I don't see why adding another soref() stops a socket
from being leaked?

I'll admit that, after clicking "reviewed" I thought the
old version might cause a leak, but discovered you
cannot "unreview" after clicking Reviewed.

Anyhow, I'm going to stick some printf()s in the code
and see what happens for both the daemon running and
daemon not running cases.

To be honest, if I understand the crash you reproduce,
it is caused by soclose() being called before svc_vc_stat()
and svc_vc_destroy(). (soclose() does sorele(), so calling it
twice isn't a good idea.)
--> How about just changing the soclose() in rpctls_rpc_failed()

to a soshutdown() and allow svc_vc_destroy_common() do
the soclose() later?

(That's a one line change from the unpatched code.)

In D57555#1319361, @rmacklem wrote:

I don't see why adding another soref() stops a socket
from being leaked?

I also removed an soref() from sys_rpctls_syscall().

I'll admit that, after clicking "reviewed" I thought the
old version might cause a leak, but discovered you
cannot "unreview" after clicking Reviewed.

I'm not in a rush. :)

Anyhow, I'm going to stick some printf()s in the code
and see what happens for both the daemon running and
daemon not running cases.

To be honest, if I understand the crash you reproduce,
it is caused by soclose() being called before svc_vc_stat()
and svc_vc_destroy(). (soclose() does sorele(), so calling it
twice isn't a good idea.)

That's right. rpctls_server() was not taking an extra ref on the xprt socket before inserting it into the upcall tree, but then if the RPC fails, it would call soclose(), releasing the xprt layer's reference.

--> How about just changing the soclose() in rpctls_rpc_failed()

to a soshutdown() and allow svc_vc_destroy_common() do
the soclose() later?

(That's a one line change from the unpatched code.)

That would revert 26ee059392094. I haven't tested the case where rpc.tlsservd is running but rpc.tlsclntd isn't. I'll give that a try too.

That said, I do somewhat prefer that we explicitly take a reference on the socket when inserting it into the upcall tree...

Ok, so now I see what you've done. You are now taking the
soref() when it is inserted in the RB tree instead of when it
is taken out.

I suspect the "else" case in rpctls_rpc_failed() needs a
sorele() along with the soshutdown(), to get rid of the
reference.

It does look like the commit I did that added the soclose()
in rpctls_rpc_failed() wasn't done correctly. (I didn't realize
that svc_vc_destroy() was going to soclose it later.)

In D57555#1319373, @rmacklem wrote:

Ok, so now I see what you've done. You are now taking the
soref() when it is inserted in the RB tree instead of when it
is taken out.

I suspect the "else" case in rpctls_rpc_failed() needs a
sorele() along with the soshutdown(), to get rid of the
reference.

Now that I am testing it, this does not seem to be needed.
(You get to the "else" case if rpc.tlsservd is running on the
server and you use your little tls_trigger.c test against it.)

It does look like the commit I did that added the soclose()
in rpctls_rpc_failed() wasn't done correctly. (I didn't realize
that svc_vc_destroy() was going to soclose it later.)

In D57555#1319744, @rmacklem wrote:

In D57555#1319373, @rmacklem wrote:

Ok, so now I see what you've done. You are now taking the
soref() when it is inserted in the RB tree instead of when it
is taken out.

I suspect the "else" case in rpctls_rpc_failed() needs a
sorele() along with the soshutdown(), to get rid of the
reference.

Now that I am testing it, this does not seem to be needed.
(You get to the "else" case if rpc.tlsservd is running on the
server and you use your little tls_trigger.c test against it.)

Right, the reference is tied to the socket's presence in the upcall_sockets tree. We should acquire a ref when adding a socket to the tree, and release it when removing from the tree. rpctls_rpc_failed() uses soclose() to release the reference. sys_rpctls_syscall() does not explicitly drop the ref after removing the socket from the tree, it implicitly transfers the reference to the file descriptor.

Ok, so I finally think I remember how this works;-)
Your patch tests out well, but I think it needs something like the following:

• sys/rpc/rpcsec_tls/rpctls_impl.c.yyy 2026-06-14 09:08:45.910463000 -0700

+++ sys/rpc/rpcsec_tls/rpctls_impl.c 2026-06-14 09:01:06.722976000 -0700
@@ -298,8 +298,18 @@ printf("rpctls_connect soref so=%p\n", so);

	stat = rpctlscd_connect_2(&arg, &res, rpctls_connect_handle);
	if (stat == RPC_SUCCESS)
		*reterr = res.reterr;

• else

+ else {
+ struct ct_data *ct = (struct ct_data *)newclient->cl_private;
+
+printf("clnt at rpctls_rpc_failed so=%p\n", so);

		rpctls_rpc_failed(&ups, so);

+ /*
+ * Since rpctls_rpc_failed() will do a soclose() if the
+ * daemon is not running, set ct_tlsstate to avoid doing
+ * a close in clnt_vc_destroy().
+ */
+ ct->ct_tlsstate = RPCTLS_INHANDSHAKE;
+ }

	/* Unblock reception. */
	CLNT_CONTROL(newclient, CLSET_BLOCKRCV, &(int){0});

So that clnt_vc_destroy() doesn't do a second soclose() when
rpc.tlsclntd is not running and a mount like..
mount -t nfs -o nfsv4,tls nfs-server:/ /mnt
is attempted on the system.

Whether
ct->ct_tlsstate = RPCTLS_INHANDSHAKE;
should have mtx_lock(&ct->ct_lock); and mtx_unlock(&ct->ct_lock);
around it, I never am sure. (jhb@ told me to do this for unconditional
assignments lonnggg ago, but.)

Other than that, I think this patch is good to go.

In D57555#1319784, @rmacklem wrote:

Ok, so I finally think I remember how this works;-)
Your patch tests out well, but I think it needs something like the following:

• sys/rpc/rpcsec_tls/rpctls_impl.c.yyy 2026-06-14 09:08:45.910463000 -0700

+++ sys/rpc/rpcsec_tls/rpctls_impl.c 2026-06-14 09:01:06.722976000 -0700
@@ -298,8 +298,18 @@ printf("rpctls_connect soref so=%p\n", so);

	stat = rpctlscd_connect_2(&arg, &res, rpctls_connect_handle);
	if (stat == RPC_SUCCESS)
		*reterr = res.reterr;

• else

+ else {
+ struct ct_data *ct = (struct ct_data *)newclient->cl_private;
+
+printf("clnt at rpctls_rpc_failed so=%p\n", so);

		rpctls_rpc_failed(&ups, so);

+ /*
+ * Since rpctls_rpc_failed() will do a soclose() if the
+ * daemon is not running, set ct_tlsstate to avoid doing
+ * a close in clnt_vc_destroy().
+ */
+ ct->ct_tlsstate = RPCTLS_INHANDSHAKE;
+ }

	/* Unblock reception. */
	CLNT_CONTROL(newclient, CLSET_BLOCKRCV, &(int){0});

So that clnt_vc_destroy() doesn't do a second soclose() when
rpc.tlsclntd is not running and a mount like..
mount -t nfs -o nfsv4,tls nfs-server:/ /mnt
is attempted on the system.

Whether
ct->ct_tlsstate = RPCTLS_INHANDSHAKE;
should have mtx_lock(&ct->ct_lock); and mtx_unlock(&ct->ct_lock);
around it, I never am sure. (jhb@ told me to do this for unconditional
assignments lonnggg ago, but.)

Other than that, I think this patch is good to go.

I think the "proper" way to spell this is just CLNT_CONTROL(newclient, CLSET_TLS, &(int){RPCTLS_FLAGS_HANDSHFAIL});.

In D57555#1319817, @markj wrote:

In D57555#1319784, @rmacklem wrote:

Other than that, I think this patch is good to go.

I think the "proper" way to spell this is just CLNT_CONTROL(newclient, CLSET_TLS, &(int){RPCTLS_FLAGS_HANDSHFAIL});.

Oops, wrong value, I meant RPCTLS_INHANDSHAKE.

In D57555#1320278, @glebius wrote:

Mark, again thanks a lot for fixing my bug that I was too slow (extremely slow!) to fix. I feel shame for that. Rick, very sorry for delaying looking into the problem for two months.

The bug was actually introduced by commit 26ee05939209, which
I did. I'll settle for embarrassed. I just hope that this is what has been
causing the crashes reported in bugzilla PR#292884 and 293127?

And, yes, thanks go to Mark for finding/fixing it.

In D57555#1320281, @rmacklem wrote:

The bug was actually introduced by commit 26ee05939209, which
I did. I'll settle for embarrassed.

Ok, then I didn't review it properly, but I should, as it was editing my code.

I just hope that this is what has been
causing the crashes reported in bugzilla PR#292884 and 293127?

I hope, too. Let's hear from people who can reproduce.