From e25d81a93fd6da4a405ba95a0cd3f06d6fa6a85b Mon Sep 17 00:00:00 2001
From: Sepherosa Ziehau <sephe@dragonflybsd.org>
Date: Tue, 22 Sep 2015 14:27:19 +0800
Subject: [PATCH] revoke(2): Use unix socket externalize code to handle revoke.

Use revoke token in shared mode in unix socket code and in exclusive
mode in fdrevoke(); mainly to make sure that all fps externalized and
marked FREVOKED will be picked up by later allproc_scan() called by
fdrevoke().

This one greatly simplies the code on unix socket side.  The original
unix socket revoke handling was also kinda broken: it tried to hold
socket reception buffer token w/ all filedesc spin lock being held.
---
 sys/kern/kern_descrip.c |  19 ++++----
 sys/kern/uipc_usrreq.c  | 127 +++---------------------------------------------
 sys/sys/thread.h        |   1 +
 sys/sys/un.h            |   1 -
 4 files changed, 16 insertions(+), 132 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 3328aae..48156a3 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -130,6 +130,8 @@ static struct spinlock filehead_spin = SPINLOCK_INITIALIZER(&filehead_spin, "fil
 static int nfiles;		/* actual number of open files */
 extern int cmask;	
 
+struct lwkt_token revoke_token = LWKT_TOKEN_INITIALIZER(revoke_token);
+
 /*
  * Fixup fd_freefile and fd_lastfile after a descriptor has been cleared.
  *
@@ -1339,7 +1341,6 @@ struct fdrevoke_info {
 	short type;
 	short unused;
 	int found;
-	int intransit;
 	struct ucred *cred;
 	struct file *nfp;
 };
@@ -1365,23 +1366,23 @@ fdrevoke(void *f_data, short f_type, struct ucred *cred)
 	 * Scan the file pointer table once.  dups do not dup file pointers,
 	 * only descriptors, so there is no leak.  Set FREVOKED on the fps
 	 * being revoked.
+	 *
+	 * Any fps sent over unix-domain sockets will be revoked by the
+	 * socket code checking for FREVOKED when the fps are externialized.
+	 * revoke_token is used to make sure that fps marked FREVOKED and
+	 * externalized will be picked up by the following allproc_scan().
 	 */
+	lwkt_gettoken(&revoke_token);
 	allfiles_scan_exclusive(fdrevoke_check_callback, &info);
+	lwkt_reltoken(&revoke_token);
 
 	/*
 	 * If any fps were marked track down the related descriptors
 	 * and close them.  Any dup()s at this point will notice
 	 * the FREVOKED already set in the fp and do the right thing.
-	 *
-	 * Any fps with non-zero msgcounts (aka sent over a unix-domain
-	 * socket) bumped the intransit counter and will require a
-	 * scan.  Races against fps leaving the socket are closed by
-	 * the socket code checking for FREVOKED.
 	 */
 	if (info.found)
 		allproc_scan(fdrevoke_proc_callback, &info);
-	if (info.intransit)
-		unp_revoke_gc(info.nfp);
 	fdrop(info.nfp);
 	return(0);
 }
@@ -1421,8 +1422,6 @@ fdrevoke_check_callback(struct file *fp, void *vinfo)
 	if (info->data == fp->f_data && info->type == fp->f_type) {
 		atomic_set_int(&fp->f_flag, FREVOKED);
 		info->found = 1;
-		if (fp->f_msgcount)
-			++info->intransit;
 	}
 	return(0);
 }
diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index 95ee1a2..86e443e 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -123,7 +123,6 @@ static void    unp_gc (void);
 static int     unp_gc_clearmarks(struct file *, void *);
 static int     unp_gc_checkmarks(struct file *, void *);
 static int     unp_gc_checkrefs(struct file *, void *);
-static int     unp_revoke_gc_check(struct file *, void *);
 static void    unp_scan (struct mbuf *, void (*)(struct file *, void *),
 				void *data);
 static void    unp_mark (struct file *, void *data);
@@ -1435,8 +1434,6 @@ unp_externalize(struct mbuf *rights)
 		/ sizeof(struct file *);
 	int f;
 
-	lwkt_gettoken(&unp_token);
-
 	/*
 	 * if the new FD's will not fit, then we free them all
 	 */
@@ -1451,7 +1448,6 @@ unp_externalize(struct mbuf *rights)
 			*rp++ = NULL;
 			unp_discard(fp, NULL);
 		}
-		lwkt_reltoken(&unp_token);
 		return (EMSGSIZE);
 	}
 
@@ -1465,7 +1461,11 @@ unp_externalize(struct mbuf *rights)
 	 * struct file pointer.
 	 * If sizeof (struct file *) is smaller than sizeof int, then
 	 * do it in reverse order.
+	 *
+	 * Hold revoke_token in 'shared' mode, so that we won't miss
+	 * the FREVOKED update on fps being externalized (fsetfd).
 	 */
+	lwkt_gettoken_shared(&revoke_token);
 	if (sizeof(struct file *) >= sizeof(int)) {
 		fdp = (int *)CMSG_DATA(cm);
 		rp = (struct file **)CMSG_DATA(cm);
@@ -1490,7 +1490,7 @@ unp_externalize(struct mbuf *rights)
 				for (i = 0; i < newfds; i++)
 					rp[i] = NULL;
 
-				lwkt_reltoken(&unp_token);
+				lwkt_reltoken(&revoke_token);
 				return (EMSGSIZE);
 			}
 			fp = rp[i];
@@ -1513,6 +1513,7 @@ unp_externalize(struct mbuf *rights)
 			*fdp-- = f;
 		}
 	}
+	lwkt_reltoken(&revoke_token);
 
 	/*
 	 * Adjust length, in case sizeof(struct file *) and sizeof(int)
@@ -1521,15 +1522,12 @@ unp_externalize(struct mbuf *rights)
 	cm->cmsg_len = CMSG_LEN(newfds * sizeof(int));
 	rights->m_len = cm->cmsg_len;
 
-	lwkt_reltoken(&unp_token);
 	return (0);
 }
 
 static void
 unp_fp_externalize(struct lwp *lp, struct file *fp, int fd)
 {
-	lwkt_gettoken(&unp_token);
-
 	if (lp) {
 		KKASSERT(fd >= 0);
 		if (fp->f_flag & FREVOKED) {
@@ -1553,8 +1551,6 @@ unp_fp_externalize(struct lwp *lp, struct file *fp, int fd)
 	unp_rights--;
 	spin_unlock(&unp_spin);
 	fdrop(fp);
-
-	lwkt_reltoken(&unp_token);
 }
 
 void
@@ -1959,117 +1955,6 @@ unp_gc_checkmarks(struct file *fp, void *data)
 }
 
 /*
- * Scan all unix domain sockets and replace any revoked file pointers
- * found with the dummy file pointer fx.  We don't worry about races
- * against file pointers being read out as those are handled in the
- * externalize code.
- */
-
-#define REVOKE_GC_MAXFILES	32
-
-struct unp_revoke_gc_info {
-	struct file	*fx;
-	struct file	*fary[REVOKE_GC_MAXFILES];
-	int		fcount;
-};
-
-void
-unp_revoke_gc(struct file *fx)
-{
-	struct unp_revoke_gc_info info;
-	int i;
-
-	lwkt_gettoken(&unp_token);
-	info.fx = fx;
-	do {
-		info.fcount = 0;
-		allfiles_scan_exclusive(unp_revoke_gc_check, &info);
-		for (i = 0; i < info.fcount; ++i)
-			unp_fp_externalize(NULL, info.fary[i], -1);
-	} while (info.fcount == REVOKE_GC_MAXFILES);
-	lwkt_reltoken(&unp_token);
-}
-
-/*
- * Check for and replace revoked descriptors.
- *
- * WARNING:  This routine is not allowed to block.
- */
-static int
-unp_revoke_gc_check(struct file *fps, void *vinfo)
-{
-	struct unp_revoke_gc_info *info = vinfo;
-	struct file *fp;
-	struct socket *so;
-	struct mbuf *m0;
-	struct mbuf *m;
-	struct file **rp;
-	struct cmsghdr *cm;
-	int i;
-	int qfds;
-
-	/*
-	 * Is this a unix domain socket with rights-passing abilities?
-	 */
-	if (fps->f_type != DTYPE_SOCKET)
-		return (0);
-	if ((so = (struct socket *)fps->f_data) == NULL)
-		return(0);
-	if (so->so_proto->pr_domain != &localdomain)
-		return(0);
-	if ((so->so_proto->pr_flags & PR_RIGHTS) == 0)
-		return(0);
-
-	/*
-	 * Scan the mbufs for control messages and replace any revoked
-	 * descriptors we find.
-	 */
-	lwkt_gettoken(&so->so_rcv.ssb_token);
-	m0 = so->so_rcv.ssb_mb;
-	while (m0) {
-		for (m = m0; m; m = m->m_next) {
-			if (m->m_type != MT_CONTROL)
-				continue;
-			if (m->m_len < sizeof(*cm))
-				continue;
-			cm = mtod(m, struct cmsghdr *);
-			if (cm->cmsg_level != SOL_SOCKET ||
-			    cm->cmsg_type != SCM_RIGHTS) {
-				continue;
-			}
-			qfds = (cm->cmsg_len - CMSG_LEN(0)) / sizeof(void *);
-			rp = (struct file **)CMSG_DATA(cm);
-			for (i = 0; i < qfds; i++) {
-				fp = rp[i];
-				if (fp->f_flag & FREVOKED) {
-					kprintf("Warning: Removing revoked fp from unix domain socket queue\n");
-					fhold(info->fx);
-					info->fx->f_msgcount++;
-					unp_rights++;
-					rp[i] = info->fx;
-					info->fary[info->fcount++] = fp;
-				}
-				if (info->fcount == REVOKE_GC_MAXFILES)
-					break;
-			}
-			if (info->fcount == REVOKE_GC_MAXFILES)
-				break;
-		}
-		m0 = m0->m_nextpkt;
-		if (info->fcount == REVOKE_GC_MAXFILES)
-			break;
-	}
-	lwkt_reltoken(&so->so_rcv.ssb_token);
-
-	/*
-	 * Stop the scan if we filled up our array.
-	 */
-	if (info->fcount == REVOKE_GC_MAXFILES)
-		return(-1);
-	return(0);
-}
-
-/*
  * Dispose of the fp's stored in a mbuf.
  *
  * The dds loop can cause additional fps to be entered onto the
diff --git a/sys/sys/thread.h b/sys/sys/thread.h
index c5a201e..2aa72fa 100644
--- a/sys/sys/thread.h
+++ b/sys/sys/thread.h
@@ -427,6 +427,7 @@ extern struct lwkt_token kvm_token;
 extern struct lwkt_token sigio_token;
 extern struct lwkt_token tty_token;
 extern struct lwkt_token vnode_token;
+extern struct lwkt_token revoke_token;
 
 /*
  * Procedures
diff --git a/sys/sys/un.h b/sys/sys/un.h
index f6717b7..5778c7f 100644
--- a/sys/sys/un.h
+++ b/sys/sys/un.h
@@ -72,7 +72,6 @@ int	uipc_usrreq (struct socket *so, int req, struct mbuf *m,
 void	uipc_ctloutput (union netmsg *msg);
 int	unp_connect2 (struct socket *so, struct socket *so2);
 void	unp_dispose (struct mbuf *m);
-void	unp_revoke_gc (struct file *fx);
 int	unp_externalize (struct mbuf *rights);
 void	unp_init (void);
 extern	struct pr_usrreqs uipc_usrreqs;
-- 
2.4.6

