Re: socket state questions

On Fri, May 1, 2026 at 8:14 AM Rick Macklem <[email protected]> wrote:
>
> 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.
I should note that the nlm is a separate story, but I don't know that
one well. (I should look at the code or check to see if all the
reporters are running it.)

The NLM was a crappy unpublished Sun protocol that should
never have been implemented in FreeBSD, so I try hard to
ignore it.;-)

rick

>
> 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