Re: [PATCH v6 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

[Jann Horn <jannh@google.com>]

On Mon, Aug 5, 2024 at 11:02 AM Marco Elver <elver@google.com> wrote:
> On Fri, 2 Aug 2024 at 22:32, Jann Horn <jannh@google.com> wrote:
> >
> > Currently, KASAN is unable to catch use-after-free in SLAB_TYPESAFE_BY_RCU
> > slabs because use-after-free is allowed within the RCU grace period by
> > design.
> >
> > Add a SLUB debugging feature which RCU-delays every individual
> > kmem_cache_free() before either actually freeing the object or handing it
> > off to KASAN, and change KASAN to poison freed objects as normal when this
> > option is enabled.
> >
> > For now I've configured Kconfig.debug to default-enable this feature in the
> > KASAN GENERIC and SW_TAGS modes; I'm not enabling it by default in HW_TAGS
> > mode because I'm not sure if it might have unwanted performance degradation
> > effects there.
> >
> > Note that this is mostly useful with KASAN in the quarantine-based GENERIC
> > mode; SLAB_TYPESAFE_BY_RCU slabs are basically always also slabs with a
> > ->ctor, and KASAN's assign_tag() currently has to assign fixed tags for
> > those, reducing the effectiveness of SW_TAGS/HW_TAGS mode.
> > (A possible future extension of this work would be to also let SLUB call
> > the ->ctor() on every allocation instead of only when the slab page is
> > allocated; then tag-based modes would be able to assign new tags on every
> > reallocation.)
> >
> > Tested-by: syzbot+263726e59eab6b442723@syzkaller.appspotmail.com
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Acked-by: Marco Elver <elver@google.com>

Thanks!

> Looks good - let's see what the fuzzers will find with it. :-)
>
> Feel free to ignore the below comments if there isn't a v+1.
[...]
> > +config SLUB_RCU_DEBUG
> > +       bool "Enable UAF detection in TYPESAFE_BY_RCU caches (for KASAN)"
> > +       depends on SLUB_DEBUG
> > +       depends on KASAN # not a real dependency; currently useless without KASAN
>
> This comment is odd. If it's useless without KASAN then it definitely
> depends on KASAN. I suppose the code compiles without KASAN, but I
> think that's secondary.

In my mind, SLUB_RCU_DEBUG is a mechanism on top of which you could
build several things - and currently only the KASAN integration is
built on top of it, but more stuff could be added in the future, like
some SLUB poisoning. So it's currently not useful unless you also
enable KASAN, but SLUB_RCU_DEBUG doesn't really depend on KASAN - it's
the other way around, KASAN has an optional dependency on
SLUB_RCU_DEBUG.

[...]
> > +#ifdef CONFIG_SLUB_RCU_DEBUG
> > +static void slab_free_after_rcu_debug(struct rcu_head *rcu_head)
> > +{
> > +       struct rcu_delayed_free *delayed_free =
> > +                       container_of(rcu_head, struct rcu_delayed_free, head);
>
> Minor: Some of these line breaks are unnecessary (kernel allows 100+
> cols) - but up to you if you want to change it.

https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
says 80 columns is still preferred unless that makes the code less
readable, that's why I'm still usually breaking at 80 columns.