From: mvs <mvs@openbsd.org>
Date: Thu, 11 Nov 2021 17:20:02 +0000 (+0000)
Subject: Use vnode(9) lock to protect `v_socket' dereference.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=5c5d25f4a2ae9342ec90ef863d12651d9c5d1619;p=openbsd

Use vnode(9) lock to protect `v_socket' dereference.

The bind(2)ed UNIX socket hat the reference from the file system layer.
When we bind(2) such socket we link it to `v_socket' of associated
vnode(9). When we connect(2) to the socket we previously bind(2)ed we
finding it by namei(9) and obtain it's reference through `v_socket'. When
we close(2) this socket we set `v_socket' of associated vnode(9) to NULL.

This time the global `unp_lock' rwlock(9) protects the whole layer and the
dereference of `v_socket'. With the upcoming fine grained locking diffs it
will be replaced by per-socket solock(). So the dereference of `v_socket'
will be unsafe because it will be unlocked and has no extra reference in
the associated file descriptor.

Actually we have vnode(9) locked while we perform unp_bind() and
unp_connect() paths so use vnode(9) lock in the unp_detach() path too when
we disconnect dying socket from the associated vnode(9). This makes
`v_socket' locking consistent because `v_socket' relies to vnode(9) layer.
Also this makes `v_socket' dereference safe for the upcoming fine grained
locking diffs.

Do `v_socket' unlinking before `unp_refs' list cleanup to prevent
concurrent connections while dying socket `so' is unlocked.

ok bluhm@
---

diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index bc36b1ebbad..0c5cbd1cdd3 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uipc_usrreq.c,v 1.154 2021/11/06 17:35:14 mvs Exp $	*/
+/*	$OpenBSD: uipc_usrreq.c,v 1.155 2021/11/11 17:20:02 mvs Exp $	*/
 /*	$NetBSD: uipc_usrreq.c,v 1.18 1996/02/09 19:00:50 christos Exp $	*/
 
 /*
@@ -485,20 +485,30 @@ void
 unp_detach(struct unpcb *unp)
 {
 	struct socket *so = unp->unp_socket;
-	struct vnode *vp = NULL;
+	struct vnode *vp = unp->unp_vnode;
 
 	rw_assert_wrlock(&unp_lock);
 
 	LIST_REMOVE(unp, unp_link);
-	if (unp->unp_vnode) {
+
+	if (vp != NULL) {
+		unp->unp_vnode = NULL;
+
 		/*
-		 * `v_socket' is only read in unp_connect and
-		 * unplock prevents concurrent access.
+		 * Enforce `i_lock' -> `unp_lock' because fifo
+		 * subsystem requires it.
 		 */
 
-		unp->unp_vnode->v_socket = NULL;
-		vp = unp->unp_vnode;
-		unp->unp_vnode = NULL;
+		sounlock(so, SL_LOCKED);
+
+		VOP_LOCK(vp, LK_EXCLUSIVE);
+		vp->v_socket = NULL;
+
+		KERNEL_LOCK();
+		vput(vp);
+		KERNEL_UNLOCK();
+
+		solock(so);
 	}
 
 	if (unp->unp_conn)
@@ -511,21 +521,6 @@ unp_detach(struct unpcb *unp)
 	pool_put(&unpcb_pool, unp);
 	if (unp_rights)
 		task_add(systqmp, &unp_gc_task);
-
-	if (vp != NULL) {
-		/*
-		 * Enforce `i_lock' -> `unplock' because fifo subsystem
-		 * requires it. The socket can't be closed concurrently
-		 * because the file descriptor reference is
-		 * still hold.
-		 */
-
-		sounlock(so, SL_LOCKED);
-		KERNEL_LOCK();
-		vrele(vp);
-		KERNEL_UNLOCK();
-		solock(so);
-	}
 }
 
 int