Page MenuHomeFreeBSD

unix: Fix a socket leak
ClosedPublic

Authored by markj on Wed, Aug 26, 8:11 PM.
Tags
None
Referenced Files
F168423789: D59201.diff
Fri, Aug 28, 5:57 AM
Unknown Object (File)
Thu, Aug 27, 2:59 PM
Unknown Object (File)
Thu, Aug 27, 2:22 PM
Unknown Object (File)
Thu, Aug 27, 9:12 AM
Unknown Object (File)
Thu, Aug 27, 7:54 AM
Unknown Object (File)
Thu, Aug 27, 6:54 AM
Unknown Object (File)
Thu, Aug 27, 5:46 AM
Unknown Object (File)
Thu, Aug 27, 5:46 AM
Subscribers

Details

Summary

When connecting a unix domain stream socket, we

  1. look up the peer (listening) socket,
  2. allocate a new socket
  3. add the new socket to the listening socket's queue

Prior to commit 26147c51546e, this sequence of operations was
synchronized by a pool mutex, also acquired in uipc_close().

After commit 26147c51546e, we drop the vnode pool lock immediately after
finding the peer socket via a filesystem lookup. This creates a window
where it's possible for a connection to add a new socket to the
listening queue after the listening queue has been aborted.

Fix the race by restoring the old behaviour of holding the pool lock
across the solisten_enqueue() call. This is a bit ugly since we need to
pass a mutex lock and a vnode through a couple of layers, but it seems
like a low-risk solution. Alternately we could add some flag to the
listening socket which indicates that no new connections are to be
accepted, but I think this will require some changes to the generic
socket code.

Reported by: pho
Fixes: 26147c51546e ("unix: pin the pathname peer by reference across the connect")

Diff Detail

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

Event Timeline

markj requested review of this revision.Wed, Aug 26, 8:11 PM
sys/kern/uipc_usrreq.c
3028

Here the sorele() is deferred to avoid releasing the last reference while holding the pool lock, as I am not sure whether that would be safe.

Ah this is indeed a bummer, but I suppose the prudent thing to do is fix the bug now. If a better solution comes along we can try that then, but until then, this is the time-tested way to do it.

This revision is now accepted and ready to land.Thu, Aug 27, 5:58 AM
sys/kern/uipc_usrreq.c
3023

So mtxpool protects the case when the connect is done through the filesystem path. What about the added case of directly passing the peer socket' fd? Or the case where the peer path is fdescfd node?

sys/kern/uipc_usrreq.c
3023

In that case, there is an additional reference on the socket via an fd, so we cannot race with uipc_close(). OTOH, the vnode does not hold a reference on the socket.

sys/kern/uipc_usrreq.c
3023

I think that both the commit message explanation of the mtxpool use, and the explanation why the peerfd is not affected, should be stated as the comments directly in code.

markj marked 2 inline comments as done.

More comments.

This revision now requires review to proceed.Thu, Aug 27, 3:36 PM
This revision is now accepted and ready to land.Thu, Aug 27, 3:40 PM
sys/kern/uipc_usrreq.c
3024

Redundant test.

sys/kern/uipc_usrreq.c
3024

Oh... not at all. Sorry for the noise.

olce requested changes to this revision.Thu, Aug 27, 4:51 PM

Please check inline comment, there may be a leak.

sys/kern/uipc_usrreq.c
3176–3177

Haven't thoroughly checked, but it seems that there's a lock + use reference leak in this branch now, as the original vput() is removed but *vpp is not set.

This revision now requires changes to proceed.Thu, Aug 27, 4:51 PM
sys/kern/uipc_usrreq.c
3176–3177

unp_dupfd_peer() unconditionally calls vput() now, isn't that sufficient?

olce added inline comments.
sys/kern/uipc_usrreq.c
3090

Change of lifecycle.

3176–3177

Oh, indeed. There's still a comment to fix there then.

This revision is now accepted and ready to land.Fri, Aug 28, 9:36 AM
This revision was automatically updated to reflect the committed changes.