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 bc36b1e..0c5cbd1 100644 (file)
--- 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