From 3812dcd3dece33af84ab6f189b678ab439d3f5fb Mon Sep 17 00:00:00 2001
From: Pawel Jakub Dawidek <pjd@FreeBSD.org>
Date: Wed, 13 Jun 2012 21:32:35 +0000
Subject: [PATCH] Allocate descriptor number in dupfdopen() itself instead of
 depending on the caller using finstall(). This saves us the filedesc
 lock/unlock cycle, fhold()/fdrop() cycle and closes a race between finstall()
 and dupfdopen().

MFC after:	1 month
---
 sys/kern/kern_descrip.c | 34 ++++++++++++++++++----------------
 sys/kern/vfs_syscalls.c | 18 +++++++-----------
 sys/sys/filedesc.h      |  4 ++--
 3 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index ef4da6e2f29f..5bad7812c393 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2588,13 +2588,13 @@ sys_flock(struct thread *td, struct flock_args *uap)
  * Duplicate the specified descriptor to a free descriptor.
  */
 int
-dupfdopen(struct thread *td, struct filedesc *fdp, int indx, int dfd, int mode, int error)
+dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode, int openerror, int *indxp)
 {
-	struct file *wfp;
 	struct file *fp;
+	int error, indx;
 
-	KASSERT(error == ENODEV || error == ENXIO,
-	    ("unexpected error %d in %s", error, __func__));
+	KASSERT(openerror == ENODEV || openerror == ENXIO,
+	    ("unexpected error %d in %s", openerror, __func__));
 
 	/*
 	 * If the to-be-dup'd fd number is greater than the allowed number
@@ -2603,11 +2603,17 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int indx, int dfd, int mode,
 	 */
 	FILEDESC_XLOCK(fdp);
 	if ((unsigned int)dfd >= fdp->fd_nfiles ||
-	    (wfp = fdp->fd_ofiles[dfd]) == NULL) {
+	    (fp = fdp->fd_ofiles[dfd]) == NULL) {
 		FILEDESC_XUNLOCK(fdp);
 		return (EBADF);
 	}
 
+	error = fdalloc(td, 0, &indx);
+	if (error != 0) {
+		FILEDESC_XUNLOCK(fdp);
+		return (error);
+	}
+
 	/*
 	 * There are two cases of interest here.
 	 *
@@ -2616,26 +2622,26 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int indx, int dfd, int mode,
 	 * For ENXIO steal away the file structure from (dfd) and store it in
 	 * (indx).  (dfd) is effectively closed by this operation.
 	 */
-	fp = fdp->fd_ofiles[indx];
-	switch (error) {
+	switch (openerror) {
 	case ENODEV:
 		/*
 		 * Check that the mode the file is being opened for is a
 		 * subset of the mode of the existing descriptor.
 		 */
-		if (((mode & (FREAD|FWRITE)) | wfp->f_flag) != wfp->f_flag) {
+		if (((mode & (FREAD|FWRITE)) | fp->f_flag) != fp->f_flag) {
+			fdunused(fdp, indx);
 			FILEDESC_XUNLOCK(fdp);
 			return (EACCES);
 		}
-		fdp->fd_ofiles[indx] = wfp;
+		fdp->fd_ofiles[indx] = fp;
 		fdp->fd_ofileflags[indx] = fdp->fd_ofileflags[dfd];
-		fhold(wfp);
+		fhold(fp);
 		break;
 	case ENXIO:
 		/*
 		 * Steal away the file pointer from dfd and stuff it into indx.
 		 */
-		fdp->fd_ofiles[indx] = wfp;
+		fdp->fd_ofiles[indx] = fp;
 		fdp->fd_ofiles[dfd] = NULL;
 		fdp->fd_ofileflags[indx] = fdp->fd_ofileflags[dfd];
 		fdp->fd_ofileflags[dfd] = 0;
@@ -2643,11 +2649,7 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int indx, int dfd, int mode,
 		break;
 	}
 	FILEDESC_XUNLOCK(fdp);
-	/*
-	 * We now own the reference to fp that the ofiles[] array used to own.
-	 * Release it.
-	 */
-	fdrop(fp, td);
+	*indxp = indx;
 	return (0);
 }
 
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 9598a16d7642..9d3d4eb57a66 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -1093,7 +1093,7 @@ kern_openat(struct thread *td, int fd, char *path, enum uio_seg pathseg,
 	struct file *fp;
 	struct vnode *vp;
 	int cmode;
-	int type, indx = -1, error, error_open;
+	int type, indx = -1, error;
 	struct flock lf;
 	struct nameidata nd;
 	int vfslocked;
@@ -1143,9 +1143,7 @@ kern_openat(struct thread *td, int fd, char *path, enum uio_seg pathseg,
 			goto success;
 
 		/*
-		 * Handle special fdopen() case. bleh. dupfdopen() is
-		 * responsible for dropping the old contents of ofiles[indx]
-		 * if it succeeds.
+		 * Handle special fdopen() case. bleh.
 		 *
 		 * Don't do this for relative (capability) lookups; we don't
 		 * understand exactly what would happen, and we don't think
@@ -1154,14 +1152,12 @@ kern_openat(struct thread *td, int fd, char *path, enum uio_seg pathseg,
 		if (nd.ni_strictrelative == 0 &&
 		    (error == ENODEV || error == ENXIO) &&
 		    td->td_dupfd >= 0) {
-			/* XXX from fdopen */
-			error_open = error;
-			if ((error = finstall(td, fp, &indx, flags)) != 0)
-				goto bad_unlocked;
-			if ((error = dupfdopen(td, fdp, indx, td->td_dupfd,
-			    flags, error_open)) == 0)
+			error = dupfdopen(td, fdp, td->td_dupfd, flags, error,
+			    &indx);
+			if (error == 0)
 				goto success;
 		}
+
 		if (error == ERESTART)
 			error = EINTR;
 		goto bad_unlocked;
@@ -4514,11 +4510,11 @@ sys_fhopen(td, uap)
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (error);
 	}
-
 	/*
 	 * An extra reference on `fp' has been held for us by
 	 * falloc_noinstall().
 	 */
+
 #ifdef INVARIANTS
 	td->td_dupfd = -1;
 #endif
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index f32165a7fa8b..bf416e91edff 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -109,8 +109,8 @@ struct filedesc_to_leader {
 struct thread;
 
 int	closef(struct file *fp, struct thread *td);
-int	dupfdopen(struct thread *td, struct filedesc *fdp, int indx, int dfd,
-	    int mode, int error);
+int	dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
+	    int openerror, int *indxp);
 int	falloc(struct thread *td, struct file **resultfp, int *resultfd,
 	    int flags);
 int	falloc_noinstall(struct thread *td, struct file **resultfp);