Re: socket state questions

On Fri, May 1, 2026 at 7:05 AM Mark Johnston <[email protected]> wrote:
>
> On Wed, Apr 29, 2026 at 07:53:18PM -0700, Rick Macklem wrote:
> > On Mon, Apr 27, 2026 at 4:15 PM Rick Macklem <[email protected]> wrote:
> > >
> > > On Mon, Apr 27, 2026 at 2:41 PM Mark Johnston <[email protected]> wrote:
> > > >
> > > > On Sun, Apr 26, 2026 at 03:24:31PM -0700, Rick Macklem wrote:
> > > > > Hi,
> > > > >
> > > > > I'm trying to figure out what might be causing the
> > > > > crashes reported by bugzilla PR#293127.
> > > > >
> > > > > When I look in svc_vc.c I find this:
> > > > > svc_vc_create(SVCPOOL *pool, struct socket *so, size_t sendsize,
> > > > >     size_t recvsize)
> > > > > {
> > > > >    SVCXPRT *xprt;
> > > > >    int error;
> > > > >
> > > > >    SOCK_LOCK(so);
> > > > >    if (so->so_state & (SS_ISCONNECTED|SS_ISDISCONNECTED)) {
> > > > >
> > > > > This "if" is testing to see if the socket should be used for listening or
> > > > > if it one returned from "accept". For NFS, this should always be true.
> > > > >
> > > > > I'm wondering about a couple of things...
> > > > > - Should SS_ISDISCONNECTING be here, as well, or will accepted
> > > > >   TCP sockets go directly from SS_ISCONNECTED to SS_ISDISCONNECTED?
> > > >
> > > > I don't think so: it's possible for TCP sockets to go through
> > > > SS_ISDISCONNECTING, but only via system calls, and soisdisconnecting()
> > > > doesn't clear SS_ISCONNECTED.
> > > >
> > > > > - Why would SS_ISDISCONNECTED be here? Can a socket go from
> > > > >   SS_ISDISCONNECTED back to SS_ISCONNECTED?
> > > > >   If not, the only reason I can think of is so that it will get closed by
> > > > >   svc_vc_destroy_common(), but then why not just soclose() it here?
> > > >
> > > > It was added there explicitly in commit dad14216507bf:
> > > >
> > > > commit dad14216507bfff12693742399c3540722178263
> > > > Author: John Baldwin <[email protected]>
> > > > Date:   Mon Apr 8 19:03:01 2013 +0000
> > > >
> > > >     Fix a potential socket leak in the NFS server.  If a client closes its
> > > >     connection after it was accepted by the userland nfsd process but before
> > > >     it was handled off to svc_vc_create() in the kernel, then svc_vc_create()
> > > >     would see it as a new listen socket and try to listen on it leaving a
> > > >     dangling reference to the socket.  Instead, check for disconnected sockets
> > > >     and treat them like a connected socket.  The call to pru_getaddr() should
> > > >     fail and cause svc_vc_create() to fail.  Note that we need to lock the
> > > >     socket to get a consistent snapshot of so_state since there is a window
> > > >     in soisdisconnected() where both flags are clear.
> > > Interesting. First off, pru_getattr() didn't exist when he did this commit.
> > > There is pru_getpeeraddr() and pru_getsockaddr(), which for TCP
> > > are set to in_getpeeraddr() and in_getsockaddr().
> > > But neither of them ever return an error.
> > > In main, they are sopeeraddr() and sosockaddr(), but they
> > > still call in_getpeeraddr() and in_getsockaddr(), which never
> > > return an error.
> > >
> > > So, something else must have been causing the failure.
> > > Maybe the old code caused sosetopt() to fail, due to a
> > > flag like INP_DROPPED or INP_TIMEWAIT being set?
> > > The new code for tcp_ctloutput() is quite different, using
> > > stuff like t_fb which I have never heard of?
> > >
> > > I wonder if the svc_vc_create() code should just check for
> > > SS_DISCONNECTED and return an error for that case
> > > or can it trust sosetopt() to fail?
> > Taking this a little further..does this look correct?
> > (If done in svc_vc_create_conn() just before it is set up for
> > processing by the kernel nfsd threads.)
> >
> > SOCK_LOCK(so);
> > SOCK_RECVBUF_LOCK(so);
> > if (!soreadable(so) && (so->so_state & SS_ISDISCONNECTED) != 0) {
> >     SOCK_RECVBUF_UNLOCK(so);
> >     SOCK_UNLOCK(so);
> >     - Just get rid of socket. (Basically return an error to the userland nfsd
> >        so that it closes the socket fd.)
> >
> > Put another way, if the socket is not soreadable() and is SS_ISDISCONNECTED,
> > is there anything to do with the socket other than close() it?
>
> Even if you check it there, there's some chance the socket would be
> disconnected immediately after the check, so I don't know that your
> proposal would change anything.  Because the transport is marked active
> at the end of svc_vc_create(), we will try to read from the socket, and
> if the socket is disconnected that ought to result in an error from
> soreceive().
>
> How exactly is svc_vc_create() getting called?  I see a few different
> call paths but I'm not sure which one(s) to focus on.
For NFS, the call is always the one in nfs_nfsdkrpc.c with a socket
that was passed to the userspace nfsd via an accept(2) and then
passed into the kernel via the nfssvc(2) syscall.

The only place that I can spot in the svc side of the krpc that seems
in any way racy is svc_vc_create(). I am now testing a patch that
does all the last stuff from xprt_register()->xprt_active() inside
the xp_lock, SOCK_LOCK and SOCK_RECV_BUFFER locks,
to try and ensure nothing weird happens while the socket is
being registered and activated. (I cannot think of a race in the
current code that would lead to these crashes, but I'm getting
desperate;-)

Since I have both SOCK_LOCK and SOCK_RECV_BUFFER_LOCK,
I thought I might as well check for the case where the socket is
already closed down and just get rid of it now instead of activating
it, etc.
--> I agree that a disconnect can happen during this activation or
     just after it, but at least the socket is registered and activated
     when I drop the above locks.

One reason why I liked having the soreadable() call is that it
checks for the socket being spliced and returns false if it is.
(Maybe I should have a separate check for isspliced()?)
Most of the churn related to sorele()s is in the socket splice
code, which I don't understand well enough to know what is
going on or even what decides that a socket should be spliced.

Since I cannot reproduce the crashes, this is all I can think of
to try. Different reporters have patches that stop the crashes
for them, but I am not convinced they fix the underlying problem.
(Basically, if you added a bunch of soref()s to the code, the
crashes would go away and the socket structure would just
get leaked, but that doesn't seem appropriate for the FreeBSD
main kernel.)

rick