Don't hold the fd-table lock across vn_open(), as opening a FIFO may block.

Having done that, dupfdopen() has to handle a possible race.

ok matthew@ krw@

2 files changed, 41 insertions, 22 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 4e46b056e04..cd6385c1066 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_descrip.c,v 1.96 2012/05/21 16:41:03 matthew Exp $	*/
+/*	$OpenBSD: kern_descrip.c,v 1.97 2012/07/08 10:55:10 guenther Exp $	*/
 /*	$NetBSD: kern_descrip.c,v 1.42 1996/03/30 22:24:38 christos Exp $	*/
 
 /*
@@ -1196,9 +1196,11 @@ filedescopen(dev_t dev, int mode, int type, struct proc *p)
  * Duplicate the specified descriptor to a free descriptor.
  */
 int
-dupfdopen(struct filedesc *fdp, int indx, int dfd, int mode)
+dupfdopen(struct proc *p, int indx, struct file *fp, int mode)
 {
+	struct filedesc *fdp = p->p_fd;
 	struct file *wfp;
+	int dfd = p->p_dupfd;
 
 	fdpassertlocked(fdp);
 
@@ -1206,10 +1208,10 @@ dupfdopen(struct filedesc *fdp, int indx, int dfd, int mode)
 	 * Assume that the filename was user-specified; applications do
 	 * not tend to open /dev/fd/# when they can just call dup()
 	 */
-	if ((curproc->p_p->ps_flags & (PS_SUGIDEXEC | PS_SUGID))) {
-		if (curproc->p_descfd == 255)
+	if ((p->p_p->ps_flags & (PS_SUGIDEXEC | PS_SUGID))) {
+		if (p->p_descfd == 255)
 			return (EPERM);
-		if (curproc->p_descfd != curproc->p_dupfd)
+		if (p->p_descfd != dfd)
 			return (EPERM);
 	}
 
@@ -1232,10 +1234,23 @@ dupfdopen(struct filedesc *fdp, int indx, int dfd, int mode)
 	if (wfp->f_count == LONG_MAX-2)
 		return (EDEADLK);
 
+	FREF(wfp);
+
+	/*
+	 * Check whether the larval fd was close behind our back.
+	 * If it was, we act like the open() completed before the
+	 * close(): fake up a closef(), to provide POSIX lock semantics,
+	 * then return success.
+	 */
+	if (fdp->fd_ofiles[indx] != fp) {
+		FREF(wfp);	/* yes, again: closef() decrements it twice */
+		closef(wfp, p);
+		return (0);
+	}
+
 	fdp->fd_ofiles[indx] = wfp;
 	fdp->fd_ofileflags[indx] = (fdp->fd_ofileflags[indx] & UF_EXCLOSE) |
 	    (fdp->fd_ofileflags[dfd] & ~UF_EXCLOSE);
-	wfp->f_count++;
 	fd_used(fdp, indx);
 	return (0);
 }
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index adfd1684156..530d29baf78 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vfs_syscalls.c,v 1.184 2012/05/21 16:41:03 matthew Exp $	*/
+/*	$OpenBSD: vfs_syscalls.c,v 1.185 2012/07/08 10:55:10 guenther Exp $	*/
 /*	$NetBSD: vfs_syscalls.c,v 1.71 1996/04/23 10:29:02 mycroft Exp $	*/
 
 /*
@@ -856,12 +856,12 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode,
 	struct nameidata nd;
 
 	fdplock(fdp);
-
 	if ((error = falloc(p, &fp, &indx)) != 0)
 		goto out;
 	flags = FFLAGS(oflags);
 	if (flags & O_CLOEXEC)
 		fdp->fd_ofileflags[indx] |= UF_EXCLOSE;
+	fdpunlock(fdp);
 
 	cmode = ((mode &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT;
 	NDINITAT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, fd, path, p);
@@ -871,19 +871,16 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode,
 		flags &= ~O_TRUNC;	/* Must do truncate ourselves */
 	}
 	if ((error = vn_open(&nd, flags, cmode)) != 0) {
+		fdplock(fdp);
 		if (error == ENODEV &&
 		    p->p_dupfd >= 0 &&			/* XXX from fdopen */
-		    (error =
-			dupfdopen(fdp, indx, p->p_dupfd, flags)) == 0) {
-			closef(fp, p);
+		    (error = dupfdopen(p, indx, fp, flags)) == 0) {
 			*retval = indx;
 			goto out;
 		}
 		if (error == ERESTART)
 			error = EINTR;
-		fdremove(fdp, indx);
-		closef(fp, p);
-		goto out;
+		goto err;
 	}
 	p->p_dupfd = 0;
 	vp = nd.ni_vp;
@@ -905,10 +902,9 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode,
 		VOP_UNLOCK(vp, 0, p);
 		error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, &lf, type);
 		if (error) {
-			/* closef will vn_close the file for us. */
-			fdremove(fdp, indx);
-			closef(fp, p);
-			goto out;
+			/* FRELE will vn_close the file for us. */
+			fdplock(fdp);
+			goto err;
 		}
 		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
 		fp->f_flag |= FHASLOCK;
@@ -927,17 +923,25 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode,
 		}
 		if (error) {
 			VOP_UNLOCK(vp, 0, p);
-			/* closef will close the file for us. */
-			fdremove(fdp, indx);
-			closef(fp, p);
-			goto out;
+			/* FRELE will close the file for us. */
+			fdplock(fdp);
+			goto err;
 		}
 	}
 	VOP_UNLOCK(vp, 0, p);
 	*retval = indx;
 	FILE_SET_MATURE(fp, p);
+	return (0);
+
+err:
+	/* only remove the fd if the file didn't change behind out back */
+	if (fdp->fd_ofiles[indx] == fp) {
+		fdremove(fdp, indx);
+		FRELE(fp, p);
+	}
 out:
 	fdpunlock(fdp);
+	FRELE(fp, p);
 	return (error);
 }