Re: socket state questions

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.