- Change falloc() to acquire an fd from the process table last so that

  it can do it w/o needing to hold the filelist_lock sx lock.
- fdalloc() doesn't need Giant to call free() anymore.  It also doesn't
  need to drop and reacquire the filedesc lock around free() now as a
  result.
- Try to make the code that copies fd tables when extending the fd table in
  fdalloc() a bit more readable by performing assignments in separate
  statements.  This is still a bit ugly though.
- Use max() instead of an if statement so to figure out the starting point
  in the search-for-a-free-fd loop in fdalloc() so it reads better next to
  the min() in the previous line.
- Don't grow nfiles in steps up to the size needed if we dup2() to some
  really large number.  Go ahead and double 'nfiles' in a loop prior
  to doing the malloc().
- malloc() doesn't need Giant now.
- Use malloc() and free() instead of MALLOC() and FREE() in fdalloc().
- Check to see if the size we are going to grow to is too big, not if the
  current size of the fd table is too big in the loop in fdalloc().  This
  means if we are out of space or if dup2() requests too high of a fd,
  then we will return an error before we go off and try to allocate some
  huge table and copy the existing table into it.
- Move all of the logic for dup'ing a file descriptor into do_dup() instead
  of putting some of it in do_dup() and duplicating other parts in four
  different places.  This makes dup(), dup2(), and fcntl(F_DUPFD) basically
  wrappers of do_dup now.  fcntl() still has an extra check since it uses
  a different error return value in one case then the other functions.
- Add a KASSERT() for an assertion that may not always be true where the
  fdcheckstd() function assumes that falloc() returns the fd requested and
  not some other fd.  I think that the assertion is always true because we
  are always single-threaded when we get to this point, but if one was
  using rfork() and another process sharing the fd table were playing with
  the fd table, there might could be a problem.
- To handle the problem of a file descriptor we are dup()'ing being closed
  out from under us in dup() in general, do_dup() now obtains a reference
  on the file in question before calling fdalloc().  If after the call to
  fdalloc() the file for the fd we are dup'ing is a different file, then
  we drop our reference on the original file and return EBADF.  This
  race was only handled in the dup2() case before and would just retry
  the operation.  The error return allows the user to know they are being
  stupid since they have a locking bug in their app instead of dup'ing
  some other descriptor and returning it to them.

Tested on:	i386, alpha, sparc64

1 files changed, 102 insertions, 108 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 5e31e95..08eeb70 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -97,8 +97,11 @@ static struct cdevsw fildesc_cdevsw = {
 	/* flags */	0,
 };
 
-static int do_dup(struct filedesc *fdp, int old, int new, register_t *retval,
-    struct thread *td);
+/* How to treat 'new' parameter when allocating a fd for do_dup(). */
+enum dup_type { DUP_VARIABLE, DUP_FIXED };
+
+static int do_dup(struct thread *td, enum dup_type type, int old, int new,
+    register_t *retval);
 static int badfo_readwrite(struct file *fp, struct uio *uio,
     struct ucred *active_cred, int flags, struct thread *td);
 static int badfo_ioctl(struct file *fp, u_long com, void *data,
@@ -166,37 +169,9 @@ dup2(td, uap)
 	struct thread *td;
 	struct dup2_args *uap;
 {
-	struct proc *p = td->td_proc;
-	register struct filedesc *fdp = td->td_proc->p_fd;
-	register u_int old = uap->from, new = uap->to;
-	int i, error;
 
-	FILEDESC_LOCK(fdp);
-retry:
-	if (old >= fdp->fd_nfiles ||
-	    fdp->fd_ofiles[old] == NULL ||
-	    new >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur ||
-	    new >= maxfilesperproc) {
-		FILEDESC_UNLOCK(fdp);
-		return (EBADF);
-	}
-	if (old == new) {
-		td->td_retval[0] = new;
-		FILEDESC_UNLOCK(fdp);
-		return (0);
-	}
-	if (new >= fdp->fd_nfiles) {
-		if ((error = fdalloc(td, new, &i))) {
-			FILEDESC_UNLOCK(fdp);
-			return (error);
-		}
-		/*
-		 * fdalloc() may block, retest everything.
-		 */
-		goto retry;
-	}
-	error = do_dup(fdp, (int)old, (int)new, td->td_retval, td);
-	return(error);
+	return (do_dup(td, DUP_FIXED, (int)uap->from, (int)uap->to,
+		    td->td_retval));
 }
 
 /*
@@ -216,23 +191,8 @@ dup(td, uap)
 	struct thread *td;
 	struct dup_args *uap;
 {
-	register struct filedesc *fdp;
-	u_int old;
-	int new, error;
 
-	old = uap->fd;
-	fdp = td->td_proc->p_fd;
-	FILEDESC_LOCK(fdp);
-	if (old >= fdp->fd_nfiles || fdp->fd_ofiles[old] == NULL) {
-		FILEDESC_UNLOCK(fdp);
-		return (EBADF);
-	}
-	if ((error = fdalloc(td, 0, &new))) {
-		FILEDESC_UNLOCK(fdp);
-		return (error);
-	}
-	error = do_dup(fdp, (int)old, new, td->td_retval, td);
-	return (error);
+	return (do_dup(td, DUP_VARIABLE, (int)uap->fd, 0, td->td_retval));
 }
 
 /*
@@ -294,7 +254,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
 	register char *pop;
 	struct vnode *vp;
 	struct flock *flp;
-	int i, tmp, error = 0, flg = F_POSIX;
+	int tmp, error = 0, flg = F_POSIX;
 	u_int newmin;
 	struct proc *leaderp;
 
@@ -312,18 +272,14 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
 
 	switch (cmd) {
 	case F_DUPFD:
+		FILEDESC_UNLOCK(fdp);
 		newmin = arg;
 		if (newmin >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur ||
 		    newmin >= maxfilesperproc) {
-			FILEDESC_UNLOCK(fdp);
 			error = EINVAL;
 			break;
 		}
-		if ((error = fdalloc(td, newmin, &i))) {
-			FILEDESC_UNLOCK(fdp);
-			break;
-		}
-		error = do_dup(fdp, fd, i, td->td_retval, td);
+		error = do_dup(td, DUP_VARIABLE, fd, newmin, td->td_retval);
 		break;
 
 	case F_GETFD:
@@ -499,16 +455,72 @@ done2:
  * filedesc must be locked, but will be unlocked as a side effect.
  */
 static int
-do_dup(fdp, old, new, retval, td)
-	register struct filedesc *fdp;
-	register int old, new;
+do_dup(td, type, old, new, retval)
+	enum dup_type type;
+	int old, new;
 	register_t *retval;
 	struct thread *td;
 {
+	register struct filedesc *fdp;
+	struct proc *p;
 	struct file *fp;
 	struct file *delfp;
+	int error, newfd;
 
-	FILEDESC_LOCK_ASSERT(fdp, MA_OWNED);
+	p = td->td_proc;
+	fdp = p->p_fd;
+
+	/*
+	 * Verify we have a valid descriptor to dup from and possibly to
+	 * dup to.
+	 */
+	FILEDESC_LOCK(fdp);
+	if (old >= fdp->fd_nfiles || fdp->fd_ofiles[old] == NULL ||
+	    new >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur ||
+	    new >= maxfilesperproc) {
+		FILEDESC_UNLOCK(fdp);
+		return (EBADF);
+	}
+	if (type == DUP_FIXED && old == new) {
+		*retval = new;
+		FILEDESC_UNLOCK(fdp);
+		return (0);
+	}
+	fp = fdp->fd_ofiles[old];
+	fhold(fp);
+
+	/*
+	 * Expand the table for the new descriptor if needed.  This may
+	 * block and drop and reacquire the filedesc lock.
+	 */
+	if (type == DUP_VARIABLE || new >= fdp->fd_nfiles) {
+		error = fdalloc(td, new, &newfd);
+		if (error) {
+			FILEDESC_UNLOCK(fdp);
+			return (error);
+		}
+	}
+	if (type == DUP_VARIABLE)
+		new = newfd;
+
+	/*
+	 * If the old file changed out from under us then treat it as a
+	 * bad file descriptor.  Userland should do its own locking to
+	 * avoid this case.
+	 */
+	if (fdp->fd_ofiles[old] != fp) {
+		if (fdp->fd_ofiles[new] == NULL) {
+			if (new < fdp->fd_freefile)
+				fdp->fd_freefile = new;
+			while (fdp->fd_lastfile > 0 &&
+			    fdp->fd_ofiles[fdp->fd_lastfile] == NULL)
+				fdp->fd_lastfile--;
+		}
+		FILEDESC_UNLOCK(fdp);
+		fdrop(fp, td);
+		return (EBADF);
+	}
+	KASSERT(old != new, ("new fd is same as old"));
 
 	/*
 	 * Save info on the descriptor being overwritten.  We have
@@ -516,6 +528,8 @@ do_dup(fdp, old, new, retval, td)
 	 * introducing an ownership race for the slot.
 	 */
 	delfp = fdp->fd_ofiles[new];
+	KASSERT(delfp == NULL || type == DUP_FIXED,
+	    ("dup() picked an open file"));
 #if 0
 	if (delfp && (fdp->fd_ofileflags[new] & UF_MAPPED))
 		(void) munmapfd(td, new);
@@ -524,15 +538,12 @@ do_dup(fdp, old, new, retval, td)
 	/*
 	 * Duplicate the source descriptor, update lastfile
 	 */
-	fp = fdp->fd_ofiles[old];
 	fdp->fd_ofiles[new] = fp;
-	fdp->fd_ofileflags[new] = fdp->fd_ofileflags[old] &~ UF_EXCLOSE;
-	fhold(fp);
+ 	fdp->fd_ofileflags[new] = fdp->fd_ofileflags[old] &~ UF_EXCLOSE;
 	if (new > fdp->fd_lastfile)
 		fdp->fd_lastfile = new;
-	*retval = new;
-
 	FILEDESC_UNLOCK(fdp);
+	*retval = new;
 
 	/*
 	 * If we dup'd over a valid file, we now own the reference to it
@@ -1013,8 +1024,7 @@ fdalloc(td, want, result)
 	lim = min((int)p->p_rlimit[RLIMIT_NOFILE].rlim_cur, maxfilesperproc);
 	for (;;) {
 		last = min(fdp->fd_nfiles, lim);
-		if ((i = want) < fdp->fd_freefile)
-			i = fdp->fd_freefile;
+		i = max(want, fdp->fd_freefile);
 		for (; i < last; i++) {
 			if (fdp->fd_ofiles[i] == NULL) {
 				fdp->fd_ofileflags[i] = 0;
@@ -1030,29 +1040,24 @@ fdalloc(td, want, result)
 		/*
 		 * No space in current array.  Expand?
 		 */
-		if (fdp->fd_nfiles >= lim)
+		if (i >= lim)
 			return (EMFILE);
 		if (fdp->fd_nfiles < NDEXTENT)
 			nfiles = NDEXTENT;
 		else
 			nfiles = 2 * fdp->fd_nfiles;
+		while (nfiles < want)
+			nfiles <<= 1;
 		FILEDESC_UNLOCK(fdp);
-		mtx_lock(&Giant);
-		MALLOC(newofile, struct file **, nfiles * OFILESIZE,
-		    M_FILEDESC, M_WAITOK);
-		mtx_unlock(&Giant);
-		FILEDESC_LOCK(fdp);
+		newofile = malloc(nfiles * OFILESIZE, M_FILEDESC, M_WAITOK);
 
 		/*
-		 * deal with file-table extend race that might have occured
-		 * when malloc was blocked.
+		 * Deal with file-table extend race that might have
+		 * occurred while filedesc was unlocked.
 		 */
+		FILEDESC_LOCK(fdp);
 		if (fdp->fd_nfiles >= nfiles) {
-			FILEDESC_UNLOCK(fdp);
-			mtx_lock(&Giant);
-			FREE(newofile, M_FILEDESC);
-			mtx_unlock(&Giant);
-			FILEDESC_LOCK(fdp);
+			free(newofile, M_FILEDESC);
 			continue;
 		}
 		newofileflags = (char *) &newofile[nfiles];
@@ -1060,11 +1065,12 @@ fdalloc(td, want, result)
 		 * Copy the existing ofile and ofileflags arrays
 		 * and zero the new portion of each array.
 		 */
-		bcopy(fdp->fd_ofiles, newofile,
-			(i = sizeof(struct file *) * fdp->fd_nfiles));
-		bzero((char *)newofile + i, nfiles * sizeof(struct file *) - i);
-		bcopy(fdp->fd_ofileflags, newofileflags,
-			(i = sizeof(char) * fdp->fd_nfiles));
+		i = fdp->fd_nfiles * sizeof(struct file *);
+		bcopy(fdp->fd_ofiles, newofile,	i);
+		bzero((char *)newofile + i,
+		    nfiles * sizeof(struct file *) - i);
+		i = fdp->fd_nfiles * sizeof(char);
+		bcopy(fdp->fd_ofileflags, newofileflags, i);
 		bzero(newofileflags + i, nfiles * sizeof(char) - i);
 		if (fdp->fd_nfiles > NDFILE)
 			oldofile = fdp->fd_ofiles;
@@ -1074,13 +1080,8 @@ fdalloc(td, want, result)
 		fdp->fd_ofileflags = newofileflags;
 		fdp->fd_nfiles = nfiles;
 		fdexpand++;
-		if (oldofile != NULL) {
-			FILEDESC_UNLOCK(fdp);
-			mtx_lock(&Giant);
-			FREE(oldofile, M_FILEDESC);
-			mtx_unlock(&Giant);
-			FILEDESC_LOCK(fdp);
-		}
+		if (oldofile != NULL)
+			free(oldofile, M_FILEDESC);
 	}
 	return (0);
 }
@@ -1143,28 +1144,26 @@ falloc(td, resultfp, resultfd)
 	 * descriptor to the list of open files at that point, otherwise
 	 * put it at the front of the list of open files.
 	 */
-	FILEDESC_LOCK(p->p_fd);
-	if ((error = fdalloc(td, 0, &i))) {
-		FILEDESC_UNLOCK(p->p_fd);
-		nfiles--;
-		sx_xunlock(&filelist_lock);
-		uma_zfree(file_zone, fp);
-		return (error);
-	}
 	fp->f_mtxp = mtx_pool_alloc();
 	fp->f_gcflag = 0;
 	fp->f_count = 1;
 	fp->f_cred = crhold(td->td_ucred);
 	fp->f_ops = &badfileops;
 	fp->f_seqcount = 1;
+	FILEDESC_LOCK(p->p_fd);
 	if ((fq = p->p_fd->fd_ofiles[0])) {
 		LIST_INSERT_AFTER(fq, fp, f_list);
 	} else {
 		LIST_INSERT_HEAD(&filehead, fp, f_list);
 	}
+	sx_xunlock(&filelist_lock);
+	if ((error = fdalloc(td, 0, &i))) {
+		FILEDESC_UNLOCK(p->p_fd);
+		fdrop(fp, td);
+		return (error);
+	}
 	p->p_fd->fd_ofiles[i] = fp;
 	FILEDESC_UNLOCK(p->p_fd);
-	sx_xunlock(&filelist_lock);
 	if (resultfp)
 		*resultfp = fp;
 	if (resultfd)
@@ -1559,13 +1558,14 @@ fdcheckstd(td)
 			error = falloc(td, &fp, &fd);
 			if (error != 0)
 				break;
+			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);
 			if (error != 0) {
 				FILEDESC_LOCK(fdp);
-				fdp->fd_ofiles[i] = NULL;
+				fdp->fd_ofiles[fd] = NULL;
 				FILEDESC_UNLOCK(fdp);
 				fdrop(fp, td);
 				break;
@@ -1578,13 +1578,7 @@ fdcheckstd(td)
 			VOP_UNLOCK(nd.ni_vp, 0, td);
 			devnull = fd;
 		} else {
-			FILEDESC_LOCK(fdp);
-			error = fdalloc(td, 0, &fd);
-			if (error != 0) {
-				FILEDESC_UNLOCK(fdp);
-				break;
-			}
-			error = do_dup(fdp, devnull, fd, &retval, td);
+			error = do_dup(td, DUP_FIXED, devnull, i, &retval);
 			if (error != 0)
 				break;
 		}