Recursive lock panic of UNP_LINK_WLOCK

[raviprakash.darbha_gmail.com]

Summary

I am hitting an race condition ( fairly hard to reproduce ) but has been reproduced in our test setup twice so far. After looking at the call trace , it seems to me that the UNP_LINK_WLOCK is being called in this particular call flow. This looks like an error condition . All of the sonewconn in the src don't have this locked so freeing it before sonewconn is called.

Created a bug on freebsd to explain the bug.

Test Plan

Unit test to recreate the situation.

[raviprakash.darbha_gmail.com]

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

[cem]

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

[cem]

sys/kern/uipc_usrreq.c

1386

I think at the very least, this needs a paired UNP_LINK_WLOCK(). Later code in this routine expects the lock to be held.

[raviprakash.darbha_gmail.com]

Adding more context to the diff with the whole file.

[cem]

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

[markj]

sys/kern/uipc_usrreq.c

1386

I think the problem with this is that we're relying on the link lock to protect against races with a concurrent detach by the listener: VOP_UNP_DETACH doesn't require the vnode lock.

It seems you need to redo the VOP_UNP_CONNECT after re-acquiring the linkage lock. That'll work given the current implementation of vop_stdunp_connect(), which has no side effects, but isn't really ideal. I think you'd also need to bump the refcnt of unp2 before dropping the lock.

[raviprakash.darbha_gmail.com]

adjusted the diff as per comments.

[markj]

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.

[raviprakash.darbha_gmail.com]

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

sys/kern/uipc_usrreq.c

1386

oops yea .. fixed that.

[markj]

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?

[markj]

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

[raviprakash.darbha_gmail.com]

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.

[pho]

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

[raviprakash.darbha_gmail.com]

This looks good. I have updated the review with the latest one. ( was having some issues with the base review being old so your patch was not applying clean to the already existing review. modified the patch a little to suit the review )

[raviprakash.darbha_gmail.com]

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