kqueue: Fix a race when adding an fd-based knote to a queue

When registering a new kevent backed by a file descriptor, we first look
up the file description with fget(), then lock the kqueue, then see if a
corresponding knote is already registered.  If not, and KN_ADD is
specified, we add the knote to the kqueue.

closefp_impl() interlocks with this process by calling knote_fdclose(),
which locks each kqueue and checks to see if the fd is registered with a
knote.  But, if userspace closes an fd while a different thread is
registering it, i.e., after fget() succeeds but before the kqueue is
locked, then we may end up with a mismatch in the knote table, where the
knote kn_fp field points to a different file description than the knote
ident.

Fix the problem by double-checking before registering a knote.  Add a
new fget_noref_unlocked() helper for this purpose.  It is a clone of
fget_noref().  We could simply use fget_noref(), but I like having an
explicit unlocked variant.

PR:		293382
Reviewed by:	kib
MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D55852

2 files changed, 30 insertions, 1 deletions

diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index e8e670d39d09..f984161bfcd6 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -28,7 +28,6 @@
  * SUCH DAMAGE.
  */
 
-#include <sys/cdefs.h>
 #include "opt_ktrace.h"
 #include "opt_kqueue.h"
 
@@ -1803,6 +1802,19 @@ findkn:
 				error = ENOMEM;
 				goto done;
 			}
+
+			/*
+			 * Now that the kqueue is locked, make sure the fd
+			 * didn't change out from under us.
+			 */
+			if (fops->f_isfd &&
+			    fget_noref_unlocked(td->td_proc->p_fd,
+			    kev->ident) != fp) {
+				KQ_UNLOCK(kq);
+				tkn = kn;
+				error = EBADF;
+				goto done;
+			}
 			kn->kn_fp = fp;
 			kn->kn_kq = kq;
 			kn->kn_fop = fops;
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index 4817855443af..c6499a18b884 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -212,6 +212,8 @@ struct filedesc_to_leader {
 
 #ifdef _KERNEL
 
+#include <machine/atomic.h>
+
 /* Operation types for kern_dup(). */
 enum {
 	FDDUP_NORMAL,		/* dup() behavior. */
@@ -303,6 +305,21 @@ int	fget_only_user(struct filedesc *fdp, int fd,
 	MPASS(refcount_load(&fp->f_count) > 0);				\
 })
 
+/*
+ * Look up a file description without requiring a lock.  In general the result
+ * may be immediately invalidated after the function returns, the caller must
+ * handle this.
+ */
+static inline struct file *
+fget_noref_unlocked(struct filedesc *fdp, int fd)
+{
+	if (__predict_false(
+	    (u_int)fd >= (u_int)atomic_load_int(&fdp->fd_nfiles)))
+		return (NULL);
+
+	return (atomic_load_ptr(&fdp->fd_ofiles[fd].fde_file));
+}
+
 /* Requires a FILEDESC_{S,X}LOCK held and returns without a ref. */
 static __inline struct file *
 fget_noref(struct filedesc *fdp, int fd)