[Bug 291005] knote_fork() NOTE_TRACK path calls kn_fop->f_event() without knlist lock

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

            Bug ID: 291005
           Summary: knote_fork() NOTE_TRACK path calls kn_fop->f_event()
                    without knlist lock
           Product: Base System
           Version: 14.3-RELEASE
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Only Me
          Priority: ---
         Component: kern
          Assignee: bugs@FreeBSD.org
          Reporter: chenqiuji666@gmail.com

This follows the earlier bug 289120
(https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=289120). In that discussion,
the GPIO maintainer confirmed that gpioc relies on kqueue calling f_event with
the knlist lock held, because the driver reads shared data under the assumption
that the callback is always invoked while locked. The kqueue(9) man page also
states that "the lock will be held over f_event calls", so this expectation is
part of the documented interface.

While re-reading sys/kern/kern_event.c (14.3-RELEASE), I noticed that
knote_fork() violates this expectation in the NOTE_TRACK path.

In the non-NOTE_TRACK path, f_event is called while the knlist lock is held:

    if ((kn->kn_sfflags & NOTE_TRACK) == 0) {
        if (kn->kn_fop->f_event(kn, NOTE_FORK))
            KNOTE_ACTIVATE(kn, 1);
        KQ_UNLOCK(kq);
        continue;
    }

But in the NOTE_TRACK case, the code explicitly drops both locks before calling
f_event:

    kn_enter_flux(kn);
    KQ_UNLOCK(kq);
    list->kl_unlock(list->kl_lockarg);

    ... two kqueue_register() calls ...

    if (kn->kn_fop->f_event(kn, NOTE_FORK)) /* f_event called unlocked */
        KNOTE_ACTIVATE(kn, 0);
    list->kl_lock(list->kl_lockarg);
    KQ_LOCK(kq);
    kn_leave_flux(kn);
    KQ_UNLOCK_FLUX(kq);

This is inconsistent with the locking pattern used everywhere else and breaks
drivers that rely on "f_event is always invoked with knlist lock held".

Suggested fix:
Re-acquire both locks before calling f_event in the NOTE_TRACK tail, and switch
KNOTE_ACTIVATE to islock = 1 since kq_lock will already be held:

    list->kl_lock(list->kl_lockarg);   /* moved up */
    KQ_LOCK(kq);                       /* moved up */
    if (kn->kn_fop->f_event(kn, NOTE_FORK))
        KNOTE_ACTIVATE(kn, 1);         /* already locked */
    kn_leave_flux(kn);
    KQ_UNLOCK_FLUX(kq);

This makes the NOTE_TRACK path follow the same locking rule as the
non-NOTE_TRACK path and avoids unexpected unlocked f_event calls.

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