unix: Fix a socket leak

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 26147c5154, this sequence of operations was
synchronized by a pool mutex, also acquired in uipc_close().

After commit 26147c5154, 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:		26147c5154 ("unix: pin the pathname peer by reference across the connect")
Reviewed by:	olce, kib, John Ericson <[email protected]>
Differential Revision:	https://reviews.freebsd.org/D59201

sys/kern/uipc_usrreq.c

@@ -295,9 +295,9 @@ static int	unp_connectat(int, struct socket *, const char *, int,
 static int	unp_connect_peer(struct socket *, struct unpcb *,
 		    struct sockaddr **, struct thread *, bool);
 static int	unp_connectat_peer(struct thread *, int, const char *,
-		    struct socket **);
+		    struct socket **, struct mtx **, struct vnode **);
 static int	unp_vnode_peer(struct vnode *, struct thread *,
-		    struct socket **);
+		    struct socket **, struct mtx **, struct vnode **);
 static void	unp_connect2(struct socket *, struct socket *, bool);
 static void	unp_disconnect(struct unpcb *unp, struct unpcb *unp2);
 static void	unp_dispose(struct socket *so);
@@ -2955,6 +2955,8 @@ unp_connectat(int fd, struct socket *so, const char *path, int len,
 	struct unpcb *unp;
 	char buf[SOCK_MAXADDRLEN];
 	struct sockaddr *sa;
+	struct mtx *mtxp;
+	struct vnode *vp;
 	int error;
 	bool connreq;
 
@@ -3006,17 +3008,36 @@ unp_connectat(int fd, struct socket *so, const char *path, int len,
 	else
 		sa = NULL;
 
-	error = unp_connectat_peer(td, fd, buf, &so2);
-	if (error != 0)
-		goto out;
-	error = unp_connect_peer(so, sotounpcb(so2), &sa, td,
-	    referenced_peerp != NULL);
-	/* Transfer the reference; the caller releases it after unlocking. */
-	if (error == 0 && referenced_peerp != NULL)
-		*referenced_peerp = so2;
-	else
-		sorele(so2);
-out:
+	/*
+	 * Find the peer socket we're connecting to, and connect to it.
+	 *
+	 * If the peer is bound to a name in the filesystem, then we hold
+	 * the vnode pool lock until the connection is established, so as to
+	 * avoid racing with a close of the peer listening socket.  If the peer
+	 * is referenced by a file descriptor, then that reference prevents the
+	 * race, so no extra synchronization is needed.
+	 */
+	error = unp_connectat_peer(td, fd, buf, &so2, &mtxp, &vp);
+	if (error == 0) {
+		error = unp_connect_peer(so, sotounpcb(so2), &sa, td,
+		    referenced_peerp != NULL);
+		if (error == 0 && referenced_peerp != NULL) {
+			*referenced_peerp = so2;
+			so2 = NULL;
+		}
+
+		/*
+		 * Release references only after the pool lock is dropped in
+		 * order to avoid potential lock ordering issues.
+		 */
+		if (mtxp != NULL) {
+			mtx_unlock(mtxp);
+			vput(vp);
+		}
+		if (so2 != NULL)
+			sorele(so2);
+	}
+
 	free(sa, M_SONAME);
 	if (__predict_false(error)) {
 		UNP_PCB_LOCK(unp);
@@ -3066,7 +3087,7 @@ unp_socket_fd_peer(struct thread *td, int fd, struct socket **so2p)
  * descriptor in td_dupfd and fails with ENODEV, the same convention open(2)
  * follows via dupfdopen() for /dev/fd.  We honour it here and resolve that
  * descriptor as the peer, so a plain connect(2) to /dev/fd/N reaches the
- * socket.  Does not consume 'vp'.
+ * socket.  Consumes the vnode reference.
  */
 static int
 unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p)
@@ -3079,13 +3100,14 @@ unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p)
 	error = VOP_OPEN(vp, FREAD, td->td_ucred, td, NULL);
 	dupfd = td->td_dupfd;
 	td->td_dupfd = 0;
-	if (error == ENODEV && dupfd >= 0)
-		return (unp_socket_fd_peer(td, dupfd, so2p));
-	if (error == 0) {
+	if (error == ENODEV && dupfd >= 0) {
+		error = unp_socket_fd_peer(td, dupfd, so2p);
+	} else if (error == 0) {
 		/* Not the dupfd convention: an openable node is not a peer. */
 		(void)VOP_CLOSE(vp, FREAD, td->td_ucred, td);
 		error = ECONNREFUSED;
 	}
+	vput(vp);
 	return (error);
 }
 
@@ -3099,16 +3121,21 @@ unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p)
  *	/dev/fd/N pathname		fdescfs names a descriptor
  *	ordinary pathname		a bound socket looked up by path
  *
- * The caller must release the returned socket with sorele().
+ * The caller must release the returned socket with sorele().  If the mutex
+ * and vnode pointers are filled, they must be released as well.
  */
 static int
 unp_connectat_peer(struct thread *td, int fd, const char *buf,
-    struct socket **so2p)
+    struct socket **so2p, struct mtx **mtxp, struct vnode **vpp)
 {
 	struct nameidata nd;
 	cap_rights_t rights;
 	int error;
 
+	*so2p = NULL;
+	*mtxp = NULL;
+	*vpp = NULL;
+
 	/*
 	 * An empty sun_path means 'fd' names the peer directly.  If it is a
 	 * socket, it is the peer, so return success (or its error) with no
@@ -3131,10 +3158,10 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf,
 	NDFREE_PNBUF(&nd);
 
 	/*
-	 * Dispatch on the resolved vnode, then drop it: for the socket cases
-	 * the returned reference keeps the peer stable, so the caller holds
-	 * no vnode lock across unp_connect_peer() (which matters for the
-	 * return_locked datagram fast path).
+	 * Find our peer socket.  If it comes from a socket on the filesystem,
+	 * then we hold on to the vnode pool lock so as to interlock with a
+	 * close of the listening socket.  If the peer comes to us via an fd,
+	 * then the fd reference itself keeps the peer stable.
 	 *
 	 * A synthetic descriptor node -- as fdescfs fabricates for a /dev/fd/N
 	 * path -- carries no type of its own (VNON); opening it yields the
@@ -3149,8 +3176,7 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf,
 	if (nd.ni_vp->v_type == VNON)
 		error = unp_dupfd_peer(nd.ni_vp, td, so2p);
 	else
-		error = unp_vnode_peer(nd.ni_vp, td, so2p);
-	vput(nd.ni_vp);
+		error = unp_vnode_peer(nd.ni_vp, td, so2p, mtxp, vpp);
 	return (error);
 }
 
@@ -3160,13 +3186,10 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf,
  * enforces the caller's authorization to reach the socket -- filesystem
  * permission (VOP_ACCESS) and MAC (mac_vnode_check_open) -- which bare readers
  * of the vnode->pcb binding, such as vfs_unp_reclaim(), deliberately skip.
- *
- * The returned reference keeps the peer stable for unp_connect_peer() once vp's
- * per-vnode binding lock is dropped, so the caller must release it with
- * sorele().  Does not consume 'vp'.
  */
 static int
-unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p)
+unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p,
+    struct mtx **mtxp, struct vnode **vpp)
 {
 	struct mtx *vplock;
 	struct unpcb *unp2;
@@ -3174,25 +3197,34 @@ unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p)
 
 	ASSERT_VOP_LOCKED(vp, __func__);
 
-	if (vp->v_type != VSOCK)
-		return (ENOTSOCK);
+	if (vp->v_type != VSOCK) {
+		error = ENOTSOCK;
+		goto fail;
+	}
 #ifdef MAC
 	error = mac_vnode_check_open(td->td_ucred, vp, VWRITE | VREAD);
 	if (error != 0)
-		return (error);
+		goto fail;
 #endif
 	error = VOP_ACCESS(vp, VWRITE, td->td_ucred, td);
 	if (error != 0)
-		return (error);
+		goto fail;
 
 	vplock = mtx_pool_find(unp_vp_mtxpool, vp);
 	mtx_lock(vplock);
 	VOP_UNP_CONNECT(vp, &unp2);
-	if (unp2 == NULL)
+	if (unp2 == NULL) {
+		mtx_unlock(vplock);
 		error = ECONNREFUSED;
-	else
-		soref(*so2p = unp2->unp_socket);
-	mtx_unlock(vplock);
+		goto fail;
+	}
+	soref(*so2p = unp2->unp_socket);
+	*mtxp = vplock;
+	*vpp = vp;
+	return (0);
+
+fail:
+	vput(vp);
 	return (error);
 }