Re: Problem with outstanding knotes and device detach - and a fix

[Jason Thorpe <thorpej%me.com@localhost>]

> On Jul 13, 2022, at 7:18 PM, Jason Thorpe <thorpej%me.com@localhost> wrote:
> 
> Ok, new version.  Main differences:

Aaaaand another new version.  This:

==> Creates a knote_impl structure that’s private to kern_event.c that has the new lock.  I took the opportunity to move kn_influx to the knote_impl as well, since absolutely no one outside of kern_event.c should touch it.  This change is ABI-compatible.

==> Improves the comments about the locking rules.

There are no functional changes since the last patch.

Index: kern/kern_event.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_event.c,v
retrieving revision 1.143
diff -u -p -r1.143 kern_event.c
--- kern/kern_event.c	13 Jul 2022 14:11:46 -0000	1.143
+++ kern/kern_event.c	17 Jul 2022 19:44:06 -0000
@@ -120,6 +120,61 @@ static void	filt_userdetach(struct knote
 static int	filt_user(struct knote *, long hint);
 static int	filt_usertouch(struct knote *, struct kevent *, long type);
 
+/*
+ * Private knote state that should never be exposed outside
+ * of kern_event.c
+ *
+ * Field locking:
+ *
+ * q	kn_kq->kq_lock
+ */
+struct knote_impl {
+	struct knote	ki_knote;
+	unsigned int	ki_influx;	/* q: in-flux counter */
+	kmutex_t	ki_foplock;	/* for kn_filterops */
+};
+
+#define	KIMPL_TO_KNOTE(kip)	(&(kip)->ki_knote)
+#define	KNOTE_TO_KIMPL(knp)	container_of((knp), struct knote_impl, ki_knote)
+
+static inline struct knote *
+knote_alloc(bool sleepok)
+{
+	struct knote_impl *ki;
+
+	ki = kmem_zalloc(sizeof(*ki), sleepok ? KM_SLEEP : KM_NOSLEEP);
+	mutex_init(&ki->ki_foplock, MUTEX_DEFAULT, IPL_NONE);
+
+	return KIMPL_TO_KNOTE(ki);
+}
+
+static inline void
+knote_free(struct knote *kn)
+{
+	struct knote_impl *ki = KNOTE_TO_KIMPL(kn);
+
+	mutex_destroy(&ki->ki_foplock);
+	kmem_free(ki, sizeof(*ki));
+}
+
+static inline void
+knote_foplock_enter(struct knote *kn)
+{
+	mutex_enter(&KNOTE_TO_KIMPL(kn)->ki_foplock);
+}
+
+static inline void
+knote_foplock_exit(struct knote *kn)
+{
+	mutex_exit(&KNOTE_TO_KIMPL(kn)->ki_foplock);
+}
+
+static inline bool
+knote_foplock_owned(struct knote *kn)
+{
+	return mutex_owned(&KNOTE_TO_KIMPL(kn)->ki_foplock);
+}
+
 static const struct fileops kqueueops = {
 	.fo_name = "kqueue",
 	.fo_read = (void *)enxio,
@@ -133,6 +188,31 @@ static const struct fileops kqueueops = 
 	.fo_restart = kqueue_restart,
 };
 
+static void
+filt_nopdetach(struct knote *kn __unused)
+{
+}
+
+static int
+filt_nopevent(struct knote *kn __unused, long hint __unused)
+{
+	return 0;
+}
+
+static const struct filterops nop_fd_filtops = {
+	.f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE,
+	.f_attach = NULL,
+	.f_detach = filt_nopdetach,
+	.f_event = filt_nopevent,
+};
+
+static const struct filterops nop_filtops = {
+	.f_flags = FILTEROP_MPSAFE,
+	.f_attach = NULL,
+	.f_detach = filt_nopdetach,
+	.f_event = filt_nopevent,
+};
+
 static const struct filterops kqread_filtops = {
 	.f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE,
 	.f_attach = NULL,
@@ -232,15 +312,41 @@ static size_t		user_kfiltersz;		/* size 
  *	-> object lock (e.g., device driver lock, &c.)
  *	-> kn_kq->kq_lock
  *
- * Locking rules:
+ * Locking rules.  ==> indicates the lock is acquired by the backing
+ * object, locks prior are acquired before calling filter ops:
+ *
+ *	f_attach: fdp->fd_lock -> knote foplock ->
+ *	  (maybe) KERNEL_LOCK ==> backing object lock
+ *
+ *	f_detach: fdp->fd_lock -> knote foplock ->
+ *	   (maybe) KERNEL_LOCK ==> backing object lock
+ *
+ *	f_event via kevent: fdp->fd_lock -> knote foplock ->
+ *	   (maybe) KERNEL_LOCK ==> backing object lock
+ *	   N.B. NOTE_SUBMIT will never be set in the "hint" argument
+ *	   in this case.
  *
- *	f_attach: fdp->fd_lock, KERNEL_LOCK
- *	f_detach: fdp->fd_lock, KERNEL_LOCK
- *	f_event(!NOTE_SUBMIT) via kevent: fdp->fd_lock, _no_ object lock
- *	f_event via knote: whatever caller guarantees
- *		Typically,	f_event(NOTE_SUBMIT) via knote: object lock
- *				f_event(!NOTE_SUBMIT) via knote: nothing,
- *					acquires/releases object lock inside.
+ *	f_event via knote (via backing object: Whatever caller guarantees.
+ *	Typically:
+ *		f_event(NOTE_SUBMIT): caller has already acquired backing
+ *		    object lock.
+ *		f_event(!NOTE_SUBMIT): caller has not acquired backing object,
+ *		    lock or has possibly acquired KERNEL_LOCK.  Backing object
+ *		    lock may or may not be acquired as-needed.
+ *	N.B. the knote foplock will **not** be acquired in this case.  The
+ *	caller guarantees that klist_fini() will not be called concurrently
+ *	with knote().
+ *
+ *	f_touch: fdp->fd_lock -> kn_kq->kq_lock (spin lock)
+ *	    N.B. knote foplock is **not** acquired in this case and
+ *	    the caller must guarantee that klist_fini() will never
+ *	    be called.  kevent_register() restricts filters that
+ *	    provide f_touch to known-safe cases.
+ *
+ *	klist_fini(): Caller must guarantee that no more knotes can
+ *	    be attached to the klist, and must **not** hold the backing
+ *	    object's lock; klist_fini() itself will acquire the foplock
+ *	    of each knote on the klist.
  *
  * Locking rules when detaching knotes:
  *
@@ -286,7 +392,7 @@ static inline bool
 kn_in_flux(struct knote *kn)
 {
 	KASSERT(mutex_owned(&kn->kn_kq->kq_lock));
-	return kn->kn_influx != 0;
+	return KNOTE_TO_KIMPL(kn)->ki_influx != 0;
 }
 
 static inline bool
@@ -298,8 +404,9 @@ kn_enter_flux(struct knote *kn)
 		return false;
 	}
 
-	KASSERT(kn->kn_influx < UINT_MAX);
-	kn->kn_influx++;
+	struct knote_impl *ki = KNOTE_TO_KIMPL(kn);
+	KASSERT(ki->ki_influx < UINT_MAX);
+	ki->ki_influx++;
 
 	return true;
 }
@@ -308,14 +415,17 @@ static inline bool
 kn_leave_flux(struct knote *kn)
 {
 	KASSERT(mutex_owned(&kn->kn_kq->kq_lock));
-	KASSERT(kn->kn_influx > 0);
-	kn->kn_influx--;
-	return kn->kn_influx == 0;
+
+	struct knote_impl *ki = KNOTE_TO_KIMPL(kn);
+	KASSERT(ki->ki_influx > 0);
+	ki->ki_influx--;
+	return ki->ki_influx == 0;
 }
 
 static void
 kn_wait_flux(struct knote *kn, bool can_loop)
 {
+	struct knote_impl *ki = KNOTE_TO_KIMPL(kn);
 	bool loop;
 
 	KASSERT(mutex_owned(&kn->kn_kq->kq_lock));
@@ -325,7 +435,7 @@ kn_wait_flux(struct knote *kn, bool can_
 	 * dropping the kq_lock.  The caller has let us know in
 	 * 'can_loop'.
 	 */
-	for (loop = true; loop && kn->kn_influx != 0; loop = can_loop) {
+	for (loop = true; loop && ki->ki_influx != 0; loop = can_loop) {
 		KQ_FLUX_WAIT(kn->kn_kq);
 	}
 }
@@ -423,33 +533,31 @@ knote_detach_quiesce(struct knote *kn)
 	return false;
 }
 
-static inline struct knote *
-knote_alloc(bool sleepok)
-{
-	struct knote *kn;
-
-	kn = kmem_zalloc(sizeof(*kn), sleepok ? KM_SLEEP : KM_NOSLEEP);
-
-	return kn;
-}
-
-static inline void
-knote_free(struct knote *kn)
-{
-	kmem_free(kn, sizeof(*kn));
-}
+/*
+ * Calls into the filterops need to be resilient against things which
+ * destroy a klist, e.g. device detach, freeing a vnode, etc., to avoid
+ * chasing garbage pointers (to data, or even potentially code in a
+ * module about to be unloaded).  To that end, we acquire the
+ * knote foplock before calling into the filter ops.  When a driver
+ * (or anything else) is tearing down its klist, klist_fini() enumerates
+ * each knote, acquires its foplock, and replaces the filterops with a
+ * nop stub, allowing knote detach (when descriptors are closed) to safely
+ * proceed.
+ */
 
 static int
 filter_attach(struct knote *kn)
 {
 	int rv;
 
+	KASSERT(knote_foplock_owned(kn));
 	KASSERT(kn->kn_fop != NULL);
 	KASSERT(kn->kn_fop->f_attach != NULL);
 
 	/*
 	 * N.B. that kn->kn_fop may change as the result of calling
-	 * f_attach().
+	 * f_attach().  After f_attach() returns, kn->kn_fop may not
+	 * be modified by code outside of klist_fini().
 	 */
 	if (kn->kn_fop->f_flags & FILTEROP_MPSAFE) {
 		rv = kn->kn_fop->f_attach(kn);
@@ -465,6 +573,8 @@ filter_attach(struct knote *kn)
 static void
 filter_detach(struct knote *kn)
 {
+
+	KASSERT(knote_foplock_owned(kn));
 	KASSERT(kn->kn_fop != NULL);
 	KASSERT(kn->kn_fop->f_detach != NULL);
 
@@ -478,10 +588,12 @@ filter_detach(struct knote *kn)
 }
 
 static int
-filter_event(struct knote *kn, long hint)
+filter_event(struct knote *kn, long hint, bool submitting)
 {
 	int rv;
 
+	/* See knote(). */
+	KASSERT(submitting || knote_foplock_owned(kn));
 	KASSERT(kn->kn_fop != NULL);
 	KASSERT(kn->kn_fop->f_event != NULL);
 
@@ -499,6 +611,19 @@ filter_event(struct knote *kn, long hint
 static int
 filter_touch(struct knote *kn, struct kevent *kev, long type)
 {
+
+	/*
+	 * XXX We cannot assert that the knote foplock is held here
+	 * XXX beause we cannot safely acquire it in all cases
+	 * XXX where "touch" will be used in kqueue_scan().  We just
+	 * XXX have to assume that f_touch will always be safe to call,
+	 * XXX and kqueue_register() allows only the two known-safe
+	 * XXX users of that op.
+	 */
+
+	KASSERT(kn->kn_fop != NULL);
+	KASSERT(kn->kn_fop->f_touch != NULL);
+
 	return kn->kn_fop->f_touch(kn, kev, type);
 }
 
@@ -1849,6 +1974,17 @@ kqueue_register(struct kqueue *kq, struc
 
 			KASSERT(kn->kn_fop != NULL);
 			/*
+			 * XXX Allow only known-safe users of f_touch.
+			 * XXX See filter_touch() for details.
+			 */
+			if (kn->kn_fop->f_touch != NULL &&
+			    kn->kn_fop != &timer_filtops &&
+			    kn->kn_fop != &user_filtops) {
+				error = ENOTSUP;
+				goto fail_ev_add;
+			}
+
+			/*
 			 * apply reference count to knote structure, and
 			 * do not release it at the end of this routine.
 			 */
@@ -1880,6 +2016,7 @@ kqueue_register(struct kqueue *kq, struc
 			 * N.B. kn->kn_fop may change as the result
 			 * of filter_attach()!
 			 */
+			knote_foplock_enter(kn);
 			error = filter_attach(kn);
 			if (error != 0) {
 #ifdef DEBUG
@@ -1893,6 +2030,7 @@ kqueue_register(struct kqueue *kq, struc
 				    ft ? ft->f_ops->fo_name : "?", error);
 #endif
 
+ fail_ev_add:
 				/*
 				 * N.B. no need to check for this note to
 				 * be in-flux, since it was never visible
@@ -1900,6 +2038,7 @@ kqueue_register(struct kqueue *kq, struc
 				 *
 				 * knote_detach() drops fdp->fd_lock
 				 */
+				knote_foplock_exit(kn);
 				mutex_enter(&kq->kq_lock);
 				KNOTE_WILLDETACH(kn);
 				KASSERT(kn_in_flux(kn) == false);
@@ -1957,6 +2096,7 @@ kqueue_register(struct kqueue *kq, struc
 	 * initial EV_ADD, but doing so will not reset any
 	 * filter which have already been triggered.
 	 */
+	knote_foplock_enter(kn);
 	kn->kn_kevent.udata = kev->udata;
 	KASSERT(kn->kn_fop != NULL);
 	if (!(kn->kn_fop->f_flags & FILTEROP_ISFD) &&
@@ -1967,6 +2107,7 @@ kqueue_register(struct kqueue *kq, struc
 		if (__predict_false(error != 0)) {
 			/* Never a new knote (which would consume newkn). */
 			KASSERT(newkn != NULL);
+			knote_foplock_exit(kn);
 			goto doneunlock;
 		}
 	} else {
@@ -1981,10 +2122,12 @@ kqueue_register(struct kqueue *kq, struc
 	 * broken and does not return an error.
 	 */
  done_ev_add:
-	rv = filter_event(kn, 0);
+	rv = filter_event(kn, 0, false);
 	if (rv)
 		knote_activate(kn);
 
+	knote_foplock_exit(kn);
+
 	/* disable knote */
 	if ((kev->flags & EV_DISABLE)) {
 		mutex_spin_enter(&kq->kq_lock);
@@ -2256,7 +2399,9 @@ relock:
 		if ((kn->kn_flags & EV_ONESHOT) == 0) {
 			mutex_spin_exit(&kq->kq_lock);
 			KASSERT(mutex_owned(&fdp->fd_lock));
-			rv = filter_event(kn, 0);
+			knote_foplock_enter(kn);
+			rv = filter_event(kn, 0, false);
+			knote_foplock_exit(kn);
 			mutex_spin_enter(&kq->kq_lock);
 			/* Re-poll if note was re-enqueued. */
 			if ((kn->kn_status & KN_QUEUED) != 0) {
@@ -2615,7 +2760,15 @@ knote(struct klist *list, long hint)
 	struct knote *kn, *tmpkn;
 
 	SLIST_FOREACH_SAFE(kn, list, kn_selnext, tmpkn) {
-		if (filter_event(kn, hint)) {
+		/*
+		 * We assume here that the backing object's lock is
+		 * already held if we're traversing the klist, and
+		 * so acquiring the knote foplock would create a
+		 * deadlock scenario.  But we also know that the klist
+		 * won't disappear on us while we're here, so not
+		 * acquiring it is safe.
+		 */
+		if (filter_event(kn, hint, true)) {
 			knote_activate(kn);
 		}
 	}
@@ -2664,7 +2817,9 @@ knote_detach(struct knote *kn, filedesc_
 
 	/* Remove from monitored object. */
 	if (dofop) {
+		knote_foplock_enter(kn);
 		filter_detach(kn);
+		knote_foplock_exit(kn);
 	}
 
 	/* Remove from descriptor table. */
@@ -2829,7 +2984,26 @@ klist_init(struct klist *list)
 void
 klist_fini(struct klist *list)
 {
-	/* Nothing, for now. */
+	struct knote *kn;
+
+	/*
+	 * Neuter all existing knotes on the klist because the list is
+	 * being destroyed.  The caller has guaranteed that no additional
+	 * knotes will be added to the list, that the backing object's
+	 * locks are not held (otherwise there is a locking order issue
+	 * with acquiring the knote foplock ), and that we can traverse
+	 * the list safely in this state.
+	 */
+	SLIST_FOREACH(kn, list, kn_selnext) {
+		knote_foplock_enter(kn);
+		KASSERT(kn->kn_fop != NULL);
+		if (kn->kn_fop->f_flags & FILTEROP_ISFD) {
+			kn->kn_fop = &nop_fd_filtops;
+		} else {
+			kn->kn_fop = &nop_filtops;
+		}
+		knote_foplock_exit(kn);
+	}
 }
 
 /*
Index: sys/event.h
===================================================================
RCS file: /cvsroot/src/sys/sys/event.h,v
retrieving revision 1.53
diff -u -p -r1.53 event.h
--- sys/event.h	13 Jul 2022 14:11:46 -0000	1.53
+++ sys/event.h	17 Jul 2022 19:44:06 -0000
@@ -262,7 +262,6 @@ struct knote {
 	struct kfilter		*kn_kfilter;
 	void 			*kn_hook;
 	int			kn_hookid;
-	unsigned int		kn_influx;	/* q: in-flux counter */
 
 #define	KN_ACTIVE	0x01U			/* event has been triggered */
 #define	KN_QUEUED	0x02U			/* event is on queue */


-- thorpej