From c48b8583f13edce285b00f3837b9ecd297e94699 Mon Sep 17 00:00:00 2001
From: Sepherosa Ziehau <sephe@dragonflybsd.org>
Date: Tue, 22 Sep 2015 22:07:36 +0800
Subject: [PATCH] kernel: Allow fhold() in allfiles_scan_exclusive()

Before this commit following race could happen, if fhold() is used
during allfiles_scan_exclusive():

      Thread1                      Thread2
         :                            :
         :                      fdrop(fp)
         :                      {
    Scan all fps                  if (f_count-- == 1) {
    (*) fhold(fp) [f_count++]         :
         :                          fo_close(fp);
    Access fp                       Unlink fp from filehead;
    fdrop(fp)                       free(fp);
         :                        }
         :                      }

This could cause use-after-free or double free (since f_count == 1
when fdrop() was called in Thread1, fp will be freed again).

We now keep extra reference on the fp, if it is on the filehead.
Once the fp reference drops to 1 and the fp is still on the filehead,
fp is then removed from the filehead and we drop the fp reference
for the filehead.  This prevents the above race from happening.
---
 sys/kern/kern_descrip.c | 42 +++++++++++++++++++++++++++++-------------
 sys/sys/file.h          |  4 ++++
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 3328aae..80a8f37 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -100,6 +100,9 @@
 #include <sys/file2.h>
 #include <sys/spinlock2.h>
 
+#define F_FLAG2_ONFILEHEAD	F_FLAG2_PRIV1
+#define F_FLAG2_ONFILEHEAD_SHIFT F_FLAG2_PRIV1_SHIFT
+
 static void fsetfd_locked(struct filedesc *fdp, struct file *fp, int fd);
 static void fdreserve_locked (struct filedesc *fdp, int fd0, int incr);
 static struct file *funsetfd_locked (struct filedesc *fdp, int fd);
@@ -1536,7 +1539,8 @@ falloc(struct lwp *lp, struct file **resultfp, int *resultfd)
 	fp = kmalloc(sizeof(struct file), M_FILE, M_WAITOK | M_ZERO);
 	spin_init(&fp->f_spin, "falloc");
 	SLIST_INIT(&fp->f_klist);
-	fp->f_count = 1;
+	fp->f_flag2 = F_FLAG2_ONFILEHEAD;
+	fp->f_count = 2; /* +1 for F_FLAG2_ONFILEHEAD. */
 	fp->f_ops = &badfileops;
 	fp->f_seqcount = 1;
 	fsetcred(fp, cred);
@@ -1728,11 +1732,8 @@ static
 void
 ffree(struct file *fp)
 {
-	KASSERT((fp->f_count == 0), ("ffree: fp_fcount not 0!"));
-	spin_lock(&filehead_spin);
-	LIST_REMOVE(fp, f_list);
-	nfiles--;
-	spin_unlock(&filehead_spin);
+	KASSERT((fp->f_count == 0), ("ffree: f_count not 0!"));
+	KASSERT((fp->f_flag2 & F_FLAG2_ONFILEHEAD) == 0, ("ffree: still on filehead"));
 	fsetcred(fp, NULL);
 	if (fp->f_nchandle.ncp)
 	    cache_drop(&fp->f_nchandle);
@@ -2428,6 +2429,8 @@ closef(struct file *fp, struct proc *p)
 void
 fhold(struct file *fp)
 {
+	/* 0->1 transition will never work */
+	KASSERT(fp->f_count > 0, ("fhold: invalid f_count %d", fp->f_count));
 	atomic_add_int(&fp->f_count, 1);
 }
 
@@ -2441,14 +2444,27 @@ fdrop(struct file *fp)
 {
 	struct flock lf;
 	struct vnode *vp;
-	int error;
+	int error, old_count;
 
-	/*
-	 * A combined fetch and subtract is needed to properly detect
-	 * 1->0 transitions, otherwise two cpus dropping from a ref
-	 * count of 2 might both try to run the 1->0 code.
-	 */
-	if (atomic_fetchadd_int(&fp->f_count, -1) > 1)
+	KASSERT(fp->f_count > 0, ("fdrop: invalid f_count %d", fp->f_count));
+	old_count = atomic_fetchadd_int(&fp->f_count, -1);
+	if (old_count == 2 &&
+	    atomic_testandclear_int(&fp->f_flag2, F_FLAG2_ONFILEHEAD_SHIFT)) {
+		/*
+		 * Take fp off the global list first, so that no one
+		 * could see it and hold it.
+		 */
+		spin_lock(&filehead_spin);
+		LIST_REMOVE(fp, f_list);
+		nfiles--;
+		spin_unlock(&filehead_spin);
+
+		/* Drop the reference for the F_FLAG2_ONFILEHEAD. */
+		KASSERT(fp->f_count > 0,
+		    ("fdrop: invalid f_count %d, off global list", fp->f_count));
+		old_count = atomic_fetchadd_int(&fp->f_count, -1);
+	}
+	if (old_count > 1)
 		return (0);
 
 	KKASSERT(SLIST_FIRST(&fp->f_klist) == NULL);
diff --git a/sys/sys/file.h b/sys/sys/file.h
index f401bff..1e63939 100644
--- a/sys/sys/file.h
+++ b/sys/sys/file.h
@@ -112,6 +112,7 @@ struct file {
 				 * count of sequential accesses -- cleared
 				 * by most seek operations.
 				 */
+	u_int	f_flag2;	/* extra flags, see F_FLAG2_ */
 	off_t	f_nextoff;	/*
 				 * offset of next expected read or write
 				 */
@@ -136,6 +137,9 @@ struct file {
 
 LIST_HEAD(filelist, file);
 
+#define F_FLAG2_PRIV1	0x0001	/* implementation private */
+#define F_FLAG2_PRIV1_SHIFT 0
+
 #endif
 
 #ifdef _KERNEL
-- 
2.4.6

