From: mvs <mvs@openbsd.org>
Date: Tue, 7 Dec 2021 01:19:47 +0000 (+0000)
Subject: Make `unp_msgcount' and `unp_file' protection with `unp_gc_lock'
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=50f1d1a66ac42be0d66a3b454ef5c067c4353ebd;p=openbsd

Make `unp_msgcount' and `unp_file' protection with `unp_gc_lock'
rwlock(9).

This save us from from races provided by unlocked access to the `f_count'
which cause false marking alive socket as dead. We always modify `f_count'
and `unp_msgcount' together so the `f_count' modification should also pass
the `unp_gc_rwlock' before `unp_msgcount' increment and after
`unp_msgcount' decrement. The locked `unp_file' assignment avoids us from
drain unp_gc() run.

This moves unp_gc() locking back when these wariables were protected with
the same lock which was taken for all garbage collector run but uses
another lock not `unp_lock'.

ok kettenis@ bluhm@
---

diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index 1c0a4454458..7cb3cc39494 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uipc_usrreq.c,v 1.158 2021/11/17 22:56:19 mvs Exp $	*/
+/*	$OpenBSD: uipc_usrreq.c,v 1.159 2021/12/07 01:19:47 mvs Exp $	*/
 /*	$NetBSD: uipc_usrreq.c,v 1.18 1996/02/09 19:00:50 christos Exp $	*/
 
 /*
@@ -960,8 +960,11 @@ restart:
 
 		fp = rp->fp;
 		rp++;
-		if ((unp = fptounp(fp)) != NULL)
-			atomic_dec_long(&unp->unp_msgcount);
+		if ((unp = fptounp(fp)) != NULL) {
+			rw_enter_write(&unp_gc_lock);
+			unp->unp_msgcount--;
+			rw_exit_write(&unp_gc_lock);
+		}
 	}
 
 	mtx_enter(&unp_rights_mtx);
@@ -1085,8 +1088,10 @@ morespace:
 		rp->flags = fdp->fd_ofileflags[fd] & UF_PLEDGED;
 		rp--;
 		if ((unp = fptounp(fp)) != NULL) {
-			atomic_inc_long(&unp->unp_msgcount);
+			rw_enter_write(&unp_gc_lock);
+			unp->unp_msgcount++;
 			unp->unp_file = fp;
+			rw_exit_write(&unp_gc_lock);
 		}
 	}
 	fdpunlock(fdp);
@@ -1099,8 +1104,11 @@ fail:
 	for ( ; i > 0; i--) {
 		rp++;
 		fp = rp->fp;
-		if ((unp = fptounp(fp)) != NULL)
-			atomic_dec_long(&unp->unp_msgcount);
+		if ((unp = fptounp(fp)) != NULL) {
+			rw_enter_write(&unp_gc_lock);
+			unp->unp_msgcount--;
+			rw_exit_write(&unp_gc_lock);
+		}
 		FRELE(fp, p);
 	}
 
@@ -1136,8 +1144,11 @@ unp_gc(void *arg __unused)
 			fp = defer->ud_fp[i].fp;
 			if (fp == NULL)
 				continue;
-			if ((unp = fptounp(fp)) != NULL)
-				atomic_dec_long(&unp->unp_msgcount);
+			if ((unp = fptounp(fp)) != NULL) {
+				rw_enter_write(&unp_gc_lock);
+				unp->unp_msgcount--;
+				rw_exit_write(&unp_gc_lock);
+			}
 			mtx_enter(&unp_rights_mtx);
 			unp_rights--;
 			mtx_leave(&unp_rights_mtx);
diff --git a/sys/sys/unpcb.h b/sys/sys/unpcb.h
index 7111ea97f54..5def517fa08 100644
--- a/sys/sys/unpcb.h
+++ b/sys/sys/unpcb.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: unpcb.h,v 1.20 2021/11/16 08:56:20 mvs Exp $	*/
+/*	$OpenBSD: unpcb.h,v 1.21 2021/12/07 01:19:47 mvs Exp $	*/
 /*	$NetBSD: unpcb.h,v 1.6 1994/06/29 06:46:08 cgd Exp $	*/
 
 /*
@@ -61,20 +61,19 @@
  *      I       immutable after creation
  *      G       unp_gc_lock
  *      U       unp_lock
- *      a       atomic
  */
 
 
 struct	unpcb {
 	struct	socket *unp_socket;	/* [I] pointer back to socket */
 	struct	vnode *unp_vnode;	/* [U] if associated with file */
-	struct	file *unp_file;		/* [a] backpointer for unp_gc() */
+	struct	file *unp_file;		/* [G] backpointer for unp_gc() */
 	struct	unpcb *unp_conn;	/* [U] control block of connected socket */
 	ino_t	unp_ino;		/* [U] fake inode number */
 	SLIST_HEAD(,unpcb) unp_refs;	/* [U] referencing socket linked list */
 	SLIST_ENTRY(unpcb) unp_nextref;	/* [U] link in unp_refs list */
 	struct	mbuf *unp_addr;		/* [U] bound address of socket */
-	long	unp_msgcount;		/* [a] references from socket rcv buf */
+	long	unp_msgcount;		/* [G] references from socket rcv buf */
 	int	unp_flags;		/* [U] this unpcb contains peer eids */
 	int	unp_gcflags;		/* [G] garbge collector flags */
 	struct	sockpeercred unp_connid;/* [U] id of peer process */