Page MenuHomeFreeBSD

unix: pin the pathname peer by reference across the connect
ClosedPublic

Authored by inquire_JohnEricson.me on Sat, Jul 25, 8:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Aug 21, 11:32 AM
Unknown Object (File)
Fri, Aug 21, 2:31 AM
Unknown Object (File)
Mon, Aug 10, 3:04 PM
Unknown Object (File)
Mon, Aug 10, 9:33 AM
Unknown Object (File)
Sun, Aug 9, 2:40 AM
Unknown Object (File)
Sat, Aug 8, 9:55 AM
Unknown Object (File)
Thu, Aug 6, 7:23 PM
Unknown Object (File)
Thu, Aug 6, 2:04 PM
Subscribers

Details

Summary

In the pathname path of unp_connectat(), take a reference on the peer
socket under the per-vnode unp_vp_mtxpool lock, drop that lock, and
vput() the vnode *before* calling unp_connect_peer(), rather than
holding the vnode lock across the connect.

unp_connect_peer() already accepts "a reference on the peer socket" as
a stability guarantee (it is exactly what the descriptor path relies on),
so this is behaviour-preserving. The payoff is that no vnode lock is held
across the connect, which removes the delicate `MPASS(!(return_locked &&
connreq))` "vput() must not sleep while the peer is locked" invariant on
the datagram fast path.

That reference then has to be released, and for the reasons described in
the code, this can only safely happen *after* the PCB is unlocked. The
boolean flag is replaced with a nullable out pointer to return the
reference to the caller so that it can carry out this responsibility.

No functional change intended.

Signed-off-by: John Ericson <John.Ericson@Obsidian.Systems>
Assisted-by: Claude Code (Claude Opus 4.8 and Fable 5)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj added inline comments.
sys/kern/uipc_usrreq.c
3035–3036

It took me some time to convince myself that this can't race with uipc_detach(), which is called only once all references to the socket are gone: uipc_close() already detaches the socket from the vnode, so it should be impossible to have unp_vnode != NULL in uipc_detach().

This revision is now accepted and ready to land.Tue, Aug 4, 9:04 PM
sys/kern/uipc_usrreq.c
3035–3036

Would you like me to add that assertion and add a comment with what you pointed out? Does seem worth reminding the next person to read the code of this invariant and what relies on it.

sys/kern/uipc_usrreq.c
3035–3036

No need thanks, I'm testing a patch already.

inquire_JohnEricson.me added inline comments.
sys/kern/uipc_usrreq.c
3035–3036

OK. Thanks for doing that.

In general, should I take "patch accepted with comment from you on a thing that could be improved" as to mean "issue is small enough that you will just handle it when landed"?

(If so, then I think on my TODO list is just test improves in D58406 + awaiting feedback on D58405?)

sys/kern/uipc_usrreq.c
3035–3036

It's not really set in stone... usually this case means, the patch looks ok to me once comments are addressed or refuted somehow. Generally I'd still expect the patch author to fix any stray bugs pointed out by comments before committing.

Here my comment was really just me thinking out loud and trying to prove that I actually thought about this patch. ;)

(If so, then I think on my TODO list is just test improves in D58406 + awaiting feedback on D58405?)

I believe so.