Re: socket state questions

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?

rick

>
> rick
>
> >
> > On a somewhat related note, should svc_vc_create() be checking for
> > errors from solisten()?
> >
> > > Thanks for any help with this, rick