falloc allocates a file structure and adds it to the file descriptor

table, acquiring the necessary locks as it works. It usually returns
two references to the new descriptor: one in the descriptor table
and one via a pointer argument.

As falloc releases the FILEDESC lock before returning, there is a
potential for a process to close the reference in the file descriptor
table before falloc's caller gets to use the file. I don't think this
can happen in practice at the moment, because Giant indirectly protects
closes.

To stop the file being completly closed in this situation, this change
makes falloc set the refcount to two when both references are returned.
This makes life easier for several of falloc's callers, because the
first thing they previously did was grab an extra reference on the
file.

Reviewed by:	iedowse
Idea run past:	jhb

8 files changed, 47 insertions, 31 deletions

diff --git a/sys/dev/streams/streams.c b/sys/dev/streams/streams.c
index 838f295..cc50c3b 100644
--- a/sys/dev/streams/streams.c
+++ b/sys/dev/streams/streams.c
@@ -189,7 +189,7 @@ static  int
 streamsopen(dev_t dev, int oflags, int devtype, struct thread *td)
 {
 	int type, protocol;
-	int fd;
+	int fd, extraref;
 	struct file *fp;
 	struct socket *so;
 	int error;
@@ -251,13 +251,21 @@ streamsopen(dev_t dev, int oflags, int devtype, struct thread *td)
 
 	if ((error = falloc(td, &fp, &fd)) != 0)
 	  return error;
+	/* An extra reference on `fp' has been held for us by falloc(). */
 
 	if ((error = socreate(family, &so, type, protocol,
 	    td->td_ucred, td)) != 0) {
 	  FILEDESC_LOCK(p->p_fd);
-	  p->p_fd->fd_ofiles[fd] = 0;
+	  /* Check the fd table entry hasn't changed since we made it. */
+	  extraref = 0;
+	  if (p->p_fd->fd_ofiles[fd] == fp) {
+	    p->p_fd->fd_ofiles[fd] = NULL;
+	    extraref = 1;
+	  }
 	  FILEDESC_UNLOCK(p->p_fd);
-	  ffree(fp);
+	  if (extraref)
+	    fdrop(fp, td);
+	  fdrop(fp, td);
 	  return error;
 	}
 
@@ -269,6 +277,7 @@ streamsopen(dev_t dev, int oflags, int devtype, struct thread *td)
 	FILEDESC_UNLOCK(p->p_fd);
 
 	(void)svr4_stream_get(fp);
+	fdrop(fp, td);
 	PROC_LOCK(p);
 	td->td_dupfd = fd;
 	PROC_UNLOCK(p);
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 1ed5140..5e00b53 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1163,6 +1163,10 @@ fdavail(td, n)
 /*
  * Create a new open file structure and allocate
  * a file decriptor for the process that refers to it.
+ * We add one reference to the file for the descriptor table
+ * and one reference for resultfp. This is to prevent us being
+ * prempted and the entry in the descriptor table closed after
+ * we release the FILEDESC lock.
  */
 int
 falloc(td, resultfp, resultfd)
@@ -1198,6 +1202,8 @@ falloc(td, resultfp, resultfd)
 	 */
 	fp->f_mtxp = mtx_pool_alloc(mtxpool_sleep);
 	fp->f_count = 1;
+	if (resultfp)
+		fp->f_count++;
 	fp->f_cred = crhold(td->td_ucred);
 	fp->f_ops = &badfileops;
 	FILEDESC_LOCK(p->p_fd);
@@ -1210,6 +1216,8 @@ falloc(td, resultfp, resultfd)
 	if ((error = fdalloc(td, 0, &i))) {
 		FILEDESC_UNLOCK(p->p_fd);
 		fdrop(fp, td);
+		if (resultfp)
+			fdrop(fp, td);
 		return (error);
 	}
 	p->p_fd->fd_ofiles[i] = fp;
@@ -1676,7 +1684,7 @@ fdcheckstd(td)
 	struct filedesc *fdp;
 	struct file *fp;
 	register_t retval;
-	int fd, i, error, flags, devnull;
+	int fd, i, error, flags, devnull, extraref;
 
 	fdp = td->td_proc->p_fd;
 	if (fdp == NULL)
@@ -1690,16 +1698,28 @@ fdcheckstd(td)
 			error = falloc(td, &fp, &fd);
 			if (error != 0)
 				break;
+			/* Note extra ref on `fp' held for us by falloc(). */
 			KASSERT(fd == i, ("oof, we didn't get our fd"));
 			NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, "/dev/null",
 			    td);
 			flags = FREAD | FWRITE;
 			error = vn_open(&nd, &flags, 0, -1);
 			if (error != 0) {
+				/*
+				 * Someone may have closed the entry in the
+				 * file descriptor table, so check it hasn't
+				 * changed before dropping the reference count.
+				 */
+				extraref = 0;
 				FILEDESC_LOCK(fdp);
-				fdp->fd_ofiles[fd] = NULL;
+				if (fdp->fd_ofiles[fd] == fp) {
+					fdp->fd_ofiles[fd] = NULL;
+					extraref = 1;
+				}
 				FILEDESC_UNLOCK(fdp);
 				fdrop(fp, td);
+				if (extraref)
+					fdrop(fp, td);
 				break;
 			}
 			NDFREE(&nd, NDF_ONLY_PNBUF);
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index 8d5345d..a9e42de 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -387,6 +387,7 @@ kqueue(struct thread *td, struct kqueue_args *uap)
 	error = falloc(td, &fp, &fd);
 	if (error)
 		goto done2;
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	kq = malloc(sizeof(struct kqueue), M_KQUEUE, M_WAITOK | M_ZERO);
 	TAILQ_INIT(&kq->kq_head);
 	FILE_LOCK(fp);
@@ -396,6 +397,7 @@ kqueue(struct thread *td, struct kqueue_args *uap)
 	TAILQ_INIT(&kq->kq_head);
 	fp->f_data = kq;
 	FILE_UNLOCK(fp);
+	fdrop(fp, td);
 	FILEDESC_LOCK(fdp);
 	td->td_retval[0] = fd;
 	if (fdp->fd_knlistsize < 0)
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index c5b39e0..ec4caf2 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -242,7 +242,7 @@ pipe(td, uap)
 		free(pmtx, M_TEMP);
 		return (error);
 	}
-	fhold(rf);
+	/* An extra reference on `rf' has been held for us by falloc(). */
 	td->td_retval[0] = fd;
 
 	/*
@@ -272,12 +272,14 @@ pipe(td, uap)
 		free(pmtx, M_TEMP);
 		return (error);
 	}
+	/* An extra reference on `wf' has been held for us by falloc(). */
 	FILE_LOCK(wf);
 	wf->f_flag = FREAD | FWRITE;
 	wf->f_type = DTYPE_PIPE;
 	wf->f_data = wpipe;
 	wf->f_ops = &pipeops;
 	FILE_UNLOCK(wf);
+	fdrop(wf, td);
 	td->td_retval[1] = fd;
 	rpipe->pipe_peer = wpipe;
 	wpipe->pipe_peer = rpipe;
diff --git a/sys/kern/uipc_syscalls.c b/sys/kern/uipc_syscalls.c
index 2d08b7e..8177942 100644
--- a/sys/kern/uipc_syscalls.c
+++ b/sys/kern/uipc_syscalls.c
@@ -116,7 +116,7 @@ socket(td, uap)
 	error = falloc(td, &fp, &fd);
 	if (error)
 		goto done2;
-	fhold(fp);
+	/* An extra reference on `fp' has been held for us by falloc(). */
 	error = socreate(uap->domain, &so, uap->type, uap->protocol,
 	    td->td_ucred, td);
 	FILEDESC_LOCK(fdp);
@@ -315,7 +315,7 @@ accept1(td, uap, compat)
 		splx(s);
 		goto done;
 	}
-	fhold(nfp);
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	td->td_retval[0] = fd;
 
 	/* connection has been removed from the listen queue */
@@ -542,16 +542,15 @@ socketpair(td, uap)
 	    td->td_ucred, td);
 	if (error)
 		goto free1;
+	/* On success extra reference to `fp1' and 'fp2' is set by falloc. */
 	error = falloc(td, &fp1, &fd);
 	if (error)
 		goto free2;
-	fhold(fp1);
 	sv[0] = fd;
 	fp1->f_data = so1;	/* so1 already has ref count */
 	error = falloc(td, &fp2, &fd);
 	if (error)
 		goto free3;
-	fhold(fp2);
 	fp2->f_data = so2;	/* so2 already has ref count */
 	sv[1] = fd;
 	error = soconnect2(so1, so2);
diff --git a/sys/kern/vfs_extattr.c b/sys/kern/vfs_extattr.c
index 5eeda6c..3783010 100644
--- a/sys/kern/vfs_extattr.c
+++ b/sys/kern/vfs_extattr.c
@@ -676,15 +676,11 @@ kern_open(struct thread *td, char *path, enum uio_seg pathseg, int flags,
 	error = falloc(td, &nfp, &indx);
 	if (error)
 		return (error);
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	fp = nfp;
 	cmode = ((mode &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT;
 	NDINIT(&nd, LOOKUP, FOLLOW, pathseg, path, td);
 	td->td_dupfd = -1;		/* XXX check for fdopen */
-	/*
-	 * Bump the ref count to prevent another process from closing
-	 * the descriptor while we are blocked in vn_open()
-	 */
-	fhold(fp);
 	error = vn_open(&nd, &flags, cmode, indx);
 	if (error) {
 
@@ -3673,13 +3669,9 @@ fhopen(td, uap)
 			vp->v_writecount--;
 		goto bad;
 	}
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	fp = nfp;	
 
-	/*
-	 * Hold an extra reference to avoid having fp ripped out 
-	 * from under us while we block in the lock op
-	 */
-	fhold(fp);
 	nfp->f_vnode = vp;
 	nfp->f_data = vp;
 	nfp->f_flag = fmode & FMASK;
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 5eeda6c..3783010 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -676,15 +676,11 @@ kern_open(struct thread *td, char *path, enum uio_seg pathseg, int flags,
 	error = falloc(td, &nfp, &indx);
 	if (error)
 		return (error);
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	fp = nfp;
 	cmode = ((mode &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT;
 	NDINIT(&nd, LOOKUP, FOLLOW, pathseg, path, td);
 	td->td_dupfd = -1;		/* XXX check for fdopen */
-	/*
-	 * Bump the ref count to prevent another process from closing
-	 * the descriptor while we are blocked in vn_open()
-	 */
-	fhold(fp);
 	error = vn_open(&nd, &flags, cmode, indx);
 	if (error) {
 
@@ -3673,13 +3669,9 @@ fhopen(td, uap)
 			vp->v_writecount--;
 		goto bad;
 	}
+	/* An extra reference on `nfp' has been held for us by falloc(). */
 	fp = nfp;	
 
-	/*
-	 * Hold an extra reference to avoid having fp ripped out 
-	 * from under us while we block in the lock op
-	 */
-	fhold(fp);
 	nfp->f_vnode = vp;
 	nfp->f_data = vp;
 	nfp->f_flag = fmode & FMASK;
diff --git a/sys/opencrypto/cryptodev.c b/sys/opencrypto/cryptodev.c
index e3bbba5..51617f5 100644
--- a/sys/opencrypto/cryptodev.c
+++ b/sys/opencrypto/cryptodev.c
@@ -751,7 +751,7 @@ cryptoioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct thread *td)
 			FREE(fcr, M_XDATA);
 			return (error);
 		}
-		fhold(f);
+		/* falloc automatically provides an extra reference to 'f'. */
 		f->f_flag = FREAD | FWRITE;
 		f->f_type = DTYPE_CRYPTO;
 		f->f_ops = &cryptofops;