https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211531 is the reference bug

Thanks! Please use arc or upload a diff with full context (generate with -U99999).

Recent editors of this file tagged for review. Feel free to remove yourselves.

In D7398#154060, @raviprakash.darbha_gmail.com wrote:

adjusted the diff as per comments.

Thanks! I think this is still not quite right: unp_refcount is protected by the PCB lock, so the "naked" operations there are not correct. You might be able to move the decrement down to where unp2's PCB lock is acquired.

Sorry for not having more to say. I'll have more time to look at this bug over the weekend. In the meantime, I wrote a quick test program that reproduces the crash: https://people.freebsd.org/~markj/unix_socket_detach.c

It takes a minute or two to hit the panic in my case. It's not clear to me if you already have a test case for the issue.

Thanks for the comments. I will be adding the PCB_LOCK to the diff and it looks fine as the order between the LINK_LOCK and PCB lock can be maintained. I will compile a patch tom after some testing . Looks something like this .

    unp2 = sotounpcb(so2);
    UNP_PCB_LOCK(unp2);
    unp2->unp_refcount++;
    UNP_PCB_UNLOCK(unp2);
    UNP_LINK_WUNLOCK();
    CURVNET_SET(so2->so_vnet);
    so3 = sonewconn(so2, 0);
    CURVNET_RESTORE();
    UNP_LINK_WLOCK();
    /* make sure v_socket is stable as we dropped the lock earlier. */
    VOP_UNP_CONNECT(vp, &so2);
} else
    so3 = NULL;
if (so3 == NULL) {
    error = ECONNREFUSED;
    goto bad2;
}
unp = sotounpcb(so);
unp3 = sotounpcb(so3);
UNP_PCB_LOCK(unp);
UNP_PCB_LOCK(unp2);
UNP_PCB_LOCK(unp3);

unp2->unp_refcount--;

In D7398#154286, @raviprakash.darbha_gmail.com wrote:

Looks something like this .

Thanks. This looks better, but still has some problems:

• Be careful not to leak the reference on unp2 if sonewconn() returns NULL.
• We need to free unp2 ourselves if the refcount drops to zero.
• Calling VOP_UNP_CONNECT() again doesn't really help with anything: it just sets so2 to vp->v_socket, which may have become NULL via VOP_UNP_DETACH (which does not require the vnode lock).

The issue is that we're potentially racing with a close of the listening socket. soclose() may be aborting incomplete sockets after sonewconn() returns a non-NULL pointer. We need to know if so2 was closed while the link lock was dropped. Otherwise so3 may have been freed by the time we execute "unp3 = sotounpcb(so3);" and we'll end up with a use-after-free. I don't see a good way to check for a close of the listening socket: uipc_close() is a no-op for listening sockets. I just added a flag in this hacky attempt at a solution: https://people.freebsd.org/~markj/patches/unp_close_race.diff

The check in my diff is pretty ugly. I also realized that dropping the link lock opens another race: now another thread could close "so" as we're trying to connect it. So we'd also need to bump unp's refcount before dropping the lock and check to see if it was closed after re-acquiring the lock. Furthermore, another consequence of dropping the lock is that we introduce the possibility of listen queue overflows for unix domain sockets. Previously, the link lock serialized the transition from SQ_INCOMP to SQ_COMP since we did:

UNP_LINK_WLOCK();
...
so3 = sonewconn(so2, 0);
...
unp_connect2(so, so3, PRU_CONNECT);
UNP_LINK_WUNLOCK();

and unp_connect2() calls soisconnected() on so3. So it was previously impossible for the listening socket's incomplete queue to overflow, because only one socket could be in it at a time.

All of these complications make me think that we should avoid dropping the lock and instead add some state to the PCB that lets us skip some of the teardown in unp_detach(), avoiding the need to (re-)acquire the link lock. Does this make sense?

Hm, ignore what I said about listen queue overflows above. I was misreading some code in sonewconn(); listen queue overflows can certainly occur with or without this change.

Here's a solution that instead adds a flag to mark children of a listening socket, and uses the flag to avoid acquiring the link lock in uipc_detach(): https://people.freebsd.org/~markj/patches/unp_close_race2.diff

Oh thanks. . I was trying to figure out the last part so that helps :) Will have a closer look at your new patch. For some reason I am unable to create a panic with the test program you provided even after leaving it to run for long enough.

Here's the version of the test program that worked for me: https://people.freebsd.org/~pho/unix_socket_detach.sh

Thanks Pho .. your test program works for me. I can hit the issue consistently .