[Bug 293382] Dead lock and kernel crash around closefp_impl

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=293382

--- Comment #24 from Paul <devgs@ukr.net> ---
Hi, 

Thanks! There was a small issue: NULL can actually be passed to `knote_free()`.
So we have adapted the patch a bit. Also, we've never discarded all previous
patches. So, to be of the same page, here's the final version that we're
testing now, on the HEAD:

diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index e8e670d39d09..bbb09789e41f 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"

@@ -230,6 +229,13 @@ static const struct filterops user_filtops = {
        .f_copy = knote_triv_copy,
 };

+#include <sys/stack.h>
+struct eknote {
+       struct knote k;
+       struct knote c;
+       struct stack s;
+};
+
 static uma_zone_t      knote_zone;
 static unsigned int __exclusive_cache_line     kq_ncallouts;
 static unsigned int    kq_calloutmax = 4 * 1024;
@@ -1803,6 +1809,19 @@ kqueue_register(struct kqueue *kq, struct kevent *kev,
struct thread *td,
                                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;
@@ -2830,19 +2849,21 @@ knote_fdclose(struct thread *td, int fd)
         * We shouldn't have to worry about new kevents appearing on fd
         * since filedesc is locked.
         */
+again:
        TAILQ_FOREACH(kq, &fdp->fd_kqlist, kq_list) {
                KQ_LOCK(kq);

-again:
                influx = 0;
                while (kq->kq_knlistsize > fd &&
                    (kn = SLIST_FIRST(&kq->kq_knlist[fd])) != NULL) {
+                       MPASS(kn->kn_kq == kq);
                        if (kn_in_flux(kn)) {
                                /* someone else might be waiting on our knote
*/
                                if (influx)
                                        wakeup(kq);
                                kq->kq_state |= KQ_FLUXWAIT;
-                               msleep(kq, &kq->kq_lock, PSOCK, "kqflxwt", 0);
+                               msleep(kq, &kq->kq_lock, PSOCK | PDROP,
+                                   "kqflxwt", 0);
                                goto again;
                        }
                        kn_enter_flux(kn);
@@ -2961,8 +2982,8 @@ static void
 knote_init(void *dummy __unused)
 {

-       knote_zone = uma_zcreate("KNOTE", sizeof(struct knote), NULL, NULL,
-           NULL, NULL, UMA_ALIGN_PTR, 0);
+       knote_zone = uma_zcreate("KNOTE", sizeof(struct eknote), NULL, NULL,
+           NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
        ast_register(TDA_KQUEUE, ASTR_ASTF_REQUIRED, 0, ast_kqueue);
        prison0.pr_klist = knlist_alloc(&prison0.pr_mtx);
 }
@@ -2971,15 +2992,24 @@ SYSINIT(knote, SI_SUB_PSEUDO, SI_ORDER_ANY, knote_init,
NULL);
 static struct knote *
 knote_alloc(int mflag)
 {
+       struct eknote *e;

-       return (uma_zalloc(knote_zone, mflag | M_ZERO));
+       e = uma_zalloc(knote_zone, mflag | M_ZERO);
+       return (&e->k);
 }

 static void
 knote_free(struct knote *kn)
 {
+       if (kn) {
+               struct eknote *e;

-       uma_zfree(knote_zone, kn);
+               e = __containerof(kn, struct eknote, k);
+               e->c = e->k;
+               stack_save(&e->s);
+               memset(&e->k, 0xdeadc0de, sizeof(e->k));
+               uma_zfree(knote_zone, e);
+       }
 }

 /*
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index 4817855443af..9ea5239afae7 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)

-- 
You are receiving this mail because:
You are the assignee for the bug.