Re: [PATCH] kfence: add kfence.fault parameter

[Alexander Potapenko <glider@google.com>]

On Tue, Mar 3, 2026 at 4:23 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, 3 Mar 2026 at 12:20, Alexander Potapenko <glider@google.com> wrote:
> >
> > > @@ -830,7 +835,8 @@ static void kfence_check_all_canary(void)
> > >  static int kfence_check_canary_callback(struct notifier_block *nb,
> > >                                         unsigned long reason, void *arg)
> > >  {
> > > -       kfence_check_all_canary();
> > > +       if (READ_ONCE(kfence_enabled))
> > > +               kfence_check_all_canary();
> >
> > By the way, should we also check for kfence_enabled when reporting errors?
>
> Not sure, I think it might be redundant - I don't see a way we should
> get to the reporting path if KFENCE is disabled. And if there
> currently is a way to get there, we should check kfence_enabled before
> (such as in this panic notifier now).
>
> > > @@ -1307,12 +1314,14 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
> > >         if (to_report) {
> > >                 raw_spin_lock_irqsave(&to_report->lock, flags);
> > >                 to_report->unprotected_page = unprotected_page;
> > > -               kfence_report_error(addr, is_write, regs, to_report, error_type);
> > > +               fault = kfence_report_error(addr, is_write, regs, to_report, error_type);
> > >                 raw_spin_unlock_irqrestore(&to_report->lock, flags);
> > >         } else {
> > >                 /* This may be a UAF or OOB access, but we can't be sure. */
> > > -               kfence_report_error(addr, is_write, regs, NULL, KFENCE_ERROR_INVALID);
> > > +               fault = kfence_report_error(addr, is_write, regs, NULL, KFENCE_ERROR_INVALID);
> > >         }
> > >
> > > +       kfence_handle_fault(fault);
> > > +
> > >         return kfence_unprotect(addr); /* Unprotect and let access proceed. */
> >
> > If kfence_handle_fault() oopses, kfence_unprotect() will never be
> > called, is that the desired behavior?
>
> It is - consider multiple kernel threads running into the same OOB or
> UAF. We should oops them all, otherwise this change is almost no
> benefit.
>
> > >         /* Require non-NULL meta, except if KFENCE_ERROR_INVALID. */
> > >         if (WARN_ON(type != KFENCE_ERROR_INVALID && !meta))
> > > -               return;
> > > +               return KFENCE_FAULT_NONE;
> >
> > We explicitly don't panic here; guess it should be fine...
>
> Yes - it's a KFENCE bug if we get here, the WARN is fine.

Reviewed-by: Alexander Potapenko <glider@google.com>