Fix a file descriptor close race in kqueue_register()

After inserting a knote, check that the associated file descriptor
still references the same file. Remove the knote if the descriptor
has changed because otherwise the kqueue becomes inconsistent with
the file descriptor table.

There is an analogous race in fcntl(F_SETLK). It is already handled,
but the code can be simplified by using the same check as in
kqueue_register().

Fix inspired by DragonFly BSD

OK mpi@, anton@

4 files changed, 49 insertions, 10 deletions

diff --git a/share/man/man9/file.9 b/share/man/man9/file.9
index d6d771088c0..e3bd8cc645d 100644
--- a/share/man/man9/file.9
+++ b/share/man/man9/file.9
@@ -1,4 +1,4 @@
-.\"     $OpenBSD: file.9,v 1.21 2019/12/31 16:59:35 visa Exp $
+.\"     $OpenBSD: file.9,v 1.22 2020/01/03 05:37:00 visa Exp $
 .\"
 .\" Copyright (c) 2002 Artur Grabowski <art@openbsd.org>
 .\" All rights reserved.
@@ -22,7 +22,7 @@
 .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd $Mdocdate: December 31 2019 $
+.Dd $Mdocdate: January 3 2020 $
 .Dt FALLOC 9
 .Os
 .Sh NAME
@@ -32,6 +32,7 @@
 .Nm FRELE ,
 .Nm fd_getfile ,
 .Nm fd_getfile_mode ,
+.Nm fd_checkclosed ,
 .Nm getsock ,
 .Nm getvnode
 .Nd an overview of file descriptor handling
@@ -51,6 +52,8 @@
 .Ft struct file *
 .Fn fd_getfile_mode "struct filedesc *fdp" "int fd" "int mode"
 .Ft int
+.Fn fd_checkclosed "struct filedesc *fdp" "int fd" "struct file *fp"
+.Ft int
 .Fn getsock "struct proc *p" "int fd" "struct file **fpp"
 .In sys/file.h
 .In sys/filedesc.h
@@ -114,6 +117,13 @@ is like
 .Fn fd_getfile
 but also checks if the file has been opened with the given mode.
 .Pp
+.Fn fd_checkclosed
+checks if file descriptor
+.Fa fd
+has been closed and no longer points to file
+.Fa fp .
+The file must have been retrieved from the descriptor previously.
+.Pp
 The files are extracted from the process context using the
 function
 .Fn getsock
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index d967d543b38..a9adcb7a83e 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_descrip.c,v 1.192 2019/08/05 08:35:59 anton Exp $	*/
+/*	$OpenBSD: kern_descrip.c,v 1.193 2020/01/03 05:37:00 visa Exp $	*/
 /*	$NetBSD: kern_descrip.c,v 1.42 1996/03/30 22:24:38 christos Exp $	*/
 
 /*
@@ -262,6 +262,18 @@ fd_getfile_mode(struct filedesc *fdp, int fd, int mode)
 	return (fp);
 }
 
+int
+fd_checkclosed(struct filedesc *fdp, int fd, struct file *fp)
+{
+	int closed;
+
+	mtx_enter(&fdp->fd_fplock);
+	KASSERT(fd < fdp->fd_nfiles);
+	closed = (fdp->fd_ofiles[fd] != fp);
+	mtx_leave(&fdp->fd_fplock);
+	return (closed);
+}
+
 /*
  * System calls on descriptors.
  */
@@ -396,7 +408,7 @@ sys_fcntl(struct proc *p, void *v, register_t *retval)
 	} */ *uap = v;
 	int fd = SCARG(uap, fd);
 	struct filedesc *fdp = p->p_fd;
-	struct file *fp, *fp2;
+	struct file *fp;
 	struct vnode *vp;
 	int i, tmp, newmin, flg = F_POSIX;
 	struct flock fl;
@@ -570,8 +582,7 @@ restart:
 			goto out;
 		}
 
-		fp2 = fd_getfile(fdp, fd);
-		if (fp != fp2) {
+		if (fd_checkclosed(fdp, fd, fp)) {
 			/*
 			 * We have lost the race with close() or dup2();
 			 * unlock, pretend that we've won the race and that
@@ -583,8 +594,6 @@ restart:
 			VOP_ADVLOCK(vp, fdp, F_UNLCK, &fl, F_POSIX);
 			fl.l_type = F_UNLCK;
 		}
-		if (fp2 != NULL)
-			FRELE(fp2, p);
 		goto out;
 
 
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index c634353caa5..3141af589ce 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_event.c,v 1.113 2019/12/31 14:09:56 visa Exp $	*/
+/*	$OpenBSD: kern_event.c,v 1.114 2020/01/03 05:37:00 visa Exp $	*/
 
 /*-
  * Copyright (c) 1999,2000,2001 Jonathan Lemon <jlemon@FreeBSD.org>
@@ -724,6 +724,25 @@ again:
 				knote_drop(kn, p);
 				goto done;
 			}
+
+			/*
+			 * If this is a file descriptor filter, check if
+			 * fd was closed while the knote was being added.
+			 * knote_fdclose() has missed kn if the function
+			 * ran before kn appeared in kq_knlist.
+			 */
+			if (fops->f_isfd &&
+			    fd_checkclosed(fdp, kev->ident, kn->kn_fp)) {
+				/*
+				 * Drop the knote silently without error
+				 * because another thread might already have
+				 * seen it. This corresponds to the insert
+				 * happening in full before the close.
+				 */
+				kn->kn_fop->f_detach(kn);
+				knote_drop(kn, p);
+				goto done;
+			}
 		} else {
 			/*
 			 * The user may change some filter values after the
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index aaadd17ebbf..3faadaee551 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: filedesc.h,v 1.41 2018/07/02 14:36:33 visa Exp $	*/
+/*	$OpenBSD: filedesc.h,v 1.42 2020/01/03 05:37:00 visa Exp $	*/
 /*	$NetBSD: filedesc.h,v 1.14 1996/04/09 20:55:28 cgd Exp $	*/
 
 /*
@@ -136,6 +136,7 @@ void	fdcloseexec(struct proc *);
 struct file *fd_iterfile(struct file *, struct proc *);
 struct file *fd_getfile(struct filedesc *, int);
 struct file *fd_getfile_mode(struct filedesc *, int, int);
+int	fd_checkclosed(struct filedesc *, int, struct file *);
 
 int	closef(struct file *, struct proc *);
 int	getsock(struct proc *, int, struct file **);