Add counting of temporary references to a struct file (as opposed to references

from fd tables and other long-lived objects). This is to avoid races between
using a file descriptor and having another process (with shared fd table)
close it. We use a separate refence count so that error values from close(2)
will be correctly returned to the caller of close(2).

The macros for those reference counts are FILE_USE(fp) and FILE_UNUSE(fp).

Make sure that the cases where closef can be called "incorrectly" (most notably
dup2(2)) are handled.

Right now only callers of closef (and {,p}read) use FILE_{,UN}USE correctly,
more fixes incoming soon.

8 files changed, 82 insertions, 34 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index ee2b3201076..a1e9ab54de1 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_descrip.c,v 1.46 2002/02/04 11:48:22 art Exp $	*/
+/*	$OpenBSD: kern_descrip.c,v 1.47 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: kern_descrip.c,v 1.42 1996/03/30 22:24:38 christos Exp $	*/
 
 /*
@@ -496,6 +496,8 @@ finishdup(p, old, new, retval)
 	 * closef can deal with that.
 	 */
 	oldfp = fdp->fd_ofiles[new];
+	if (oldfp != NULL)
+		FILE_USE(oldfp);
 
 	fp = fdp->fd_ofiles[old];
 	if (fp->f_count == LONG_MAX-2)
@@ -541,6 +543,7 @@ fdrelease(p, fd)
 	fp = *fpp;
 	if (fp == NULL)
 		return (EBADF);
+	FILE_USE(fp);
 	*fpp = NULL;
 	fdp->fd_ofileflags[fd] = 0;
 	fd_unused(fdp, fd);
@@ -831,6 +834,8 @@ ffree(fp)
 	crfree(fp->f_cred);
 #ifdef DIAGNOSTIC
 	fp->f_count = 0;
+	if (fp->f_usecount != 0)
+		panic("ffree: usecount != 0");
 #endif
 	nfiles--;
 	pool_put(&file_pool, fp);
@@ -989,6 +994,7 @@ fdfree(p)
 	for (i = fdp->fd_lastfile; i >= 0; i--, fpp++) {
 		fp = *fpp;
 		if (fp != NULL) {
+			FILE_USE(fp);
 			*fpp = NULL;
 			(void) closef(fp, p);
 		}
@@ -1016,11 +1022,11 @@ fdfree(p)
  * Decrement reference count on file structure.
  * Note: p may be NULL when closing a file
  * that was being passed in a message.
+ *
+ * The fp must have its usecount bumped and will be FILE_UNUSEd here.
  */
 int
-closef(fp, p)
-	register struct file *fp;
-	register struct proc *p;
+closef(struct file *fp, struct proc *p)
 {
 	struct vnode *vp;
 	struct flock lf;
@@ -1028,6 +1034,7 @@ closef(fp, p)
 
 	if (fp == NULL)
 		return (0);
+
 	/*
 	 * POSIX record locking dictates that any close releases ALL
 	 * locks owned by this process.  This is handled by setting
@@ -1044,10 +1051,33 @@ closef(fp, p)
 		vp = (struct vnode *)fp->f_data;
 		(void) VOP_ADVLOCK(vp, (caddr_t)p, F_UNLCK, &lf, F_POSIX);
 	}
-	if (--fp->f_count > 0)
+
+	/*
+	 * Some files passed to this function could be accessed
+	 * without a FILE_IS_USABLE check (and in some cases it's perfectly
+	 * legal), we must beware of files where someone already won the
+	 * race to FIF_WANTCLOSE.
+	 */
+	if ((fp->f_iflags & FIF_WANTCLOSE) != 0) {
+		FILE_UNUSE(fp);
+		return (0);
+	}
+
+	if (--fp->f_count > 0) {
+		FILE_UNUSE(fp);
 		return (0);
+	}
+
+#ifdef DIAGNOSTIC
 	if (fp->f_count < 0)
 		panic("closef: count < 0");
+#endif
+
+	/* Wait for the last usecount to drain. */
+	fp->f_iflags |= FIF_WANTCLOSE;
+	while (fp->f_usecount > 1)
+		tsleep(&fp->f_usecount, PRIBIO, "closef", 0);
+
 	if ((fp->f_flag & FHASLOCK) && fp->f_type == DTYPE_VNODE) {
 		lf.l_whence = SEEK_SET;
 		lf.l_start = 0;
@@ -1060,6 +1090,7 @@ closef(fp, p)
 		error = (*fp->f_ops->fo_close)(fp, p);
 	else
 		error = 0;
+	fp->f_usecount--;
 	ffree(fp);
 	return (error);
 }
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index 5529931a986..04163b72d60 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_event.c,v 1.14 2002/02/01 15:32:43 art Exp $	*/
+/*	$OpenBSD: kern_event.c,v 1.15 2002/02/05 16:02:27 art Exp $	*/
 
 /*-
  * Copyright (c) 1999,2000,2001 Jonathan Lemon <jlemon@FreeBSD.org>
@@ -331,7 +331,7 @@ sys_kevent(struct proc *p, void *v, register_t *retval)
 	} */ *uap = v;
 	struct kevent *kevp;
 	struct kqueue *kq;
-	struct file *fp = NULL;
+	struct file *fp;
 	struct timespec ts;
 	int i, n, nerrors, error;
 
@@ -339,7 +339,7 @@ sys_kevent(struct proc *p, void *v, register_t *retval)
 	    (fp->f_type != DTYPE_KQUEUE))
 		return (EBADF);
 
-	fp->f_count++;
+	FILE_USE(fp);
 
 	if (SCARG(uap, timeout) != NULL) {
 		error = copyin(SCARG(uap, timeout), &ts, sizeof(ts));
@@ -390,8 +390,7 @@ sys_kevent(struct proc *p, void *v, register_t *retval)
 			    SCARG(uap, timeout), p, &n);
 	*retval = n;
  done:
-	if (fp != NULL)
-		closef(fp, p);
+	FILE_UNUSE(fp);
 	return (error);
 }
 
@@ -423,6 +422,7 @@ kqueue_register(struct kqueue *kq, struct kevent *kev, struct proc *p)
 		/* validate descriptor */
 		if ((fp = fd_getfile(fdp, kev->ident)) == NULL)
 			return (EBADF);
+		FILE_USE(fp);
 		fp->f_count++;
 
 		if (kev->ident < fdp->fd_knlistsize) {
@@ -469,6 +469,7 @@ kqueue_register(struct kqueue *kq, struct kevent *kev, struct proc *p)
 			 * apply reference count to knote structure, and
 			 * do not release it at the end of this routine.
 			 */
+			FILE_UNUSE(fp);
 			fp = NULL;
 
 			kn->kn_sfflags = kev->fflags;
@@ -727,6 +728,7 @@ kqueue_close(struct file *fp, struct proc *p)
 		while (kn != NULL) {
 			kn0 = SLIST_NEXT(kn, kn_link);
 			if (kq == kn->kn_kq) {
+				FILE_USE(kn->kn_fp);
 				kn->kn_fop->f_detach(kn);
 				closef(kn->kn_fp, p);
 				knote_free(kn);
@@ -868,8 +870,10 @@ knote_drop(struct knote *kn, struct proc *p)
 	SLIST_REMOVE(list, kn, knote, kn_link);
 	if (kn->kn_status & KN_QUEUED)
 		knote_dequeue(kn);
-	if (kn->kn_fop->f_isfd)
+	if (kn->kn_fop->f_isfd) {
+		FILE_USE(kn->kn_fp);
 		closef(kn->kn_fp, p);
+	}
 	knote_free(kn);
 }
 
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index 575e4fc58c0..fd32b2a9959 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sys_generic.c,v 1.32 2002/02/02 16:05:58 art Exp $	*/
+/*	$OpenBSD: sys_generic.c,v 1.33 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: sys_generic.c,v 1.24 1996/03/29 00:25:32 cgd Exp $	*/
 
 /*
@@ -91,6 +91,8 @@ sys_read(p, v, retval)
 	if ((fp->f_flag & FREAD) == 0)
 		return (EBADF);
 
+	FILE_USE(fp);
+
 	/* dofileread() will unuse the descriptor for us */
 	return (dofileread(p, fd, fp, SCARG(uap, buf), SCARG(uap, nbyte),
 	    &fp->f_offset, retval));
@@ -152,9 +154,7 @@ dofileread(p, fd, fp, buf, nbyte, offset, retval)
 #endif
 	*retval = cnt;
  out:
-#if notyet
-	FILE_UNUSE(fp, p);
-#endif
+	FILE_UNUSE(fp);
 	return (error);
 }
 
diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index 0d04d9a80d5..80b05f89225 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uipc_usrreq.c,v 1.16 2002/02/02 16:05:58 art Exp $	*/
+/*	$OpenBSD: uipc_usrreq.c,v 1.17 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: uipc_usrreq.c,v 1.18 1996/02/09 19:00:50 christos Exp $	*/
 
 /*
@@ -887,6 +887,7 @@ unp_gc()
 		if (fp->f_count == fp->f_msgcount && !(fp->f_flag & FMARK)) {
 			*fpp++ = fp;
 			nunref++;
+			FILE_USE(fp);
 			fp->f_count++;
 		}
 	}
@@ -894,7 +895,7 @@ unp_gc()
 	        if ((*fpp)->f_type == DTYPE_SOCKET && (*fpp)->f_data != NULL)
 		        sorflush((struct socket *)(*fpp)->f_data);
 	for (i = nunref, fpp = extra_ref; --i >= 0; ++fpp)
-		(void) closef(*fpp, (struct proc *)0);
+		(void) closef(*fpp, NULL);
 	free((caddr_t)extra_ref, M_FILE);
 	unp_gcing = 0;
 }
@@ -956,7 +957,8 @@ unp_discard(fp)
 	struct file *fp;
 {
 
+	FILE_USE(fp);
 	fp->f_msgcount--;
 	unp_rights--;
-	(void) closef(fp, (struct proc *)0);
+	(void) closef(fp, NULL);
 }
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 47a76c13f37..ef9c29fa8c1 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vfs_syscalls.c,v 1.87 2002/02/04 11:43:16 art Exp $	*/
+/*	$OpenBSD: vfs_syscalls.c,v 1.88 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: vfs_syscalls.c,v 1.71 1996/04/23 10:29:02 mycroft Exp $	*/
 
 /*
@@ -874,6 +874,8 @@ sys_open(p, v, retval)
 	if ((error = falloc(p, &fp, &indx)) != 0)
 		return (error);
 
+	FILE_USE(fp);
+
 	flags = FFLAGS(SCARG(uap, flags));
 	cmode = ((SCARG(uap, mode) &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT;
 	NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
@@ -918,6 +920,7 @@ sys_open(p, v, retval)
 		error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, &lf, type);
 		if (error) {
 			(void) vn_close(vp, fp->f_flag, fp->f_cred, p);
+			FILE_UNUSE(fp);
 			ffree(fp);
 			fdremove(fdp, indx);
 			return (error);
@@ -941,6 +944,7 @@ sys_open(p, v, retval)
 		if (error) {
 			VOP_UNLOCK(vp, 0, p);
 			(void) vn_close(vp, fp->f_flag, fp->f_cred, p);
+			FILE_UNUSE(fp);
 			ffree(fp);
 			fdremove(fdp, indx);
 			return (error);
@@ -949,6 +953,7 @@ sys_open(p, v, retval)
 	VOP_UNLOCK(vp, 0, p);
 	*retval = indx;
 	FILE_SET_MATURE(fp);
+	FILE_UNUSE(fp);
 	return (0);
 }
 
@@ -2639,17 +2644,19 @@ sys_pread(p, v, retval)
 	struct file *fp;
 	struct vnode *vp;
 	off_t offset;
-	int error, fd = SCARG(uap, fd);
+	int fd = SCARG(uap, fd);
 
 	if ((fp = fd_getfile(fdp, fd)) == NULL)
 		return (EBADF);
 	if ((fp->f_flag & FREAD) == 0)
 		return (EBADF);
 
+	FILE_USE(fp);
+
 	vp = (struct vnode *)fp->f_data;
 	if (fp->f_type != DTYPE_VNODE || vp->v_type == VFIFO) {
-		error = ESPIPE;
-		goto out;
+		FILE_UNUSE(fp);
+		return (ESPIPE);
 	}
 
 	offset = SCARG(uap, offset);
@@ -2657,12 +2664,6 @@ sys_pread(p, v, retval)
 	/* dofileread() will unuse the descriptor for us */
 	return (dofileread(p, fd, fp, SCARG(uap, buf), SCARG(uap, nbyte),
 	    &offset, retval));
-
- out:
-#if notyet
-	FILE_UNUSE(fp, p);
-#endif
-	return (error);
 }
 
 /*
diff --git a/sys/miscfs/portal/portal_vfsops.c b/sys/miscfs/portal/portal_vfsops.c
index a5beda20981..16d18417044 100644
--- a/sys/miscfs/portal/portal_vfsops.c
+++ b/sys/miscfs/portal/portal_vfsops.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: portal_vfsops.c,v 1.9 2001/02/20 01:50:10 assar Exp $	*/
+/*	$OpenBSD: portal_vfsops.c,v 1.10 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: portal_vfsops.c,v 1.14 1996/02/09 22:40:41 christos Exp $	*/
 
 /*
@@ -187,12 +187,13 @@ portal_unmount(mp, mntflags, p)
 	 * daemon to wake up, and then the accept will get ECONNABORTED
 	 * which it interprets as a request to go and bury itself.
 	 */
+	FILE_USE(VFSTOPORTAL(mp)->pm_server);
 	soshutdown((struct socket *) VFSTOPORTAL(mp)->pm_server->f_data, 2);
 	/*
 	 * Discard reference to underlying file.  Must call closef because
 	 * this may be the last reference.
 	 */
-	closef(VFSTOPORTAL(mp)->pm_server, (struct proc *) 0);
+	closef(VFSTOPORTAL(mp)->pm_server, NULL);
 	/*
 	 * Finally, throw away the portalmount structure
 	 */
diff --git a/sys/nfs/nfs_syscalls.c b/sys/nfs/nfs_syscalls.c
index ae14c56ca07..1843336d880 100644
--- a/sys/nfs/nfs_syscalls.c
+++ b/sys/nfs/nfs_syscalls.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nfs_syscalls.c,v 1.25 2002/01/20 23:51:29 hugh Exp $	*/
+/*	$OpenBSD: nfs_syscalls.c,v 1.26 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: nfs_syscalls.c,v 1.19 1996/02/18 11:53:52 fvdl Exp $	*/
 
 /*
@@ -781,11 +781,12 @@ nfsrv_zapsock(slp)
 	slp->ns_flag &= ~SLP_ALLFLAGS;
 	fp = slp->ns_fp;
 	if (fp) {
-		slp->ns_fp = (struct file *)0;
+		FILE_USE(fp);
+		slp->ns_fp = NULL;
 		so = slp->ns_so;
 		so->so_upcall = NULL;
 		soshutdown(so, 2);
-		closef(fp, (struct proc *)0);
+		closef(fp, NULL);
 		if (slp->ns_nam)
 			MFREE(slp->ns_nam, m);
 		m_freem(slp->ns_raw);
diff --git a/sys/sys/file.h b/sys/sys/file.h
index 96f100d8f3a..c8b37713515 100644
--- a/sys/sys/file.h
+++ b/sys/sys/file.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: file.h,v 1.14 2001/10/31 01:36:18 art Exp $	*/
+/*	$OpenBSD: file.h,v 1.15 2002/02/05 16:02:27 art Exp $	*/
 /*	$NetBSD: file.h,v 1.11 1995/03/26 20:24:13 jtc Exp $	*/
 
 /*
@@ -83,6 +83,7 @@ struct file {
 	off_t	f_offset;
 	caddr_t	f_data;		/* private data */
 	int	f_iflags;	/* internal flags */
+	int	f_usecount;	/* number of users (temporary references). */
 };
 
 #define FIF_WANTCLOSE		0x01	/* a close is waiting for usecount */
@@ -95,6 +96,13 @@ struct file {
 	(fp)->f_iflags &= ~FIF_LARVAL;	\
 } while (0)
 
+#define FILE_USE(fp) do { (fp)->f_usecount++; } while (0)
+#define FILE_UNUSE(fp) do {				\
+	--(fp)->f_usecount;				\
+	if (((fp)->f_iflags & FIF_WANTCLOSE) != 0)	\
+		wakeup(&(fp)->f_usecount);		\
+} while (0)
+
 LIST_HEAD(filelist, file);
 extern struct filelist filehead;	/* head of list of open files */
 extern int maxfiles;			/* kernel limit on number of open files */