From 3ac4a2f623c16e7c9121ca6b05af56ae167dfdfd Mon Sep 17 00:00:00 2001 From: Visa Hankala Date: Fri, 3 Jan 2020 05:37:01 +0000 Subject: 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@ --- share/man/man9/file.9 | 14 ++++++++++++-- sys/kern/kern_descrip.c | 21 +++++++++++++++------ sys/kern/kern_event.c | 21 ++++++++++++++++++++- sys/sys/filedesc.h | 3 ++- 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 .\" 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 @@ -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 **); -- cgit v1.2.3