Message-ID: <CAG48ez1guHcQaZtGoap7MG1sac5F3PmMA7XKUH03pEaibvaFJw@mail.gmail.com>
Date: Fri, 2 Aug 2024 11:09:52 +0200
From: Jann Horn <jannh@...gle.com>
To: Andrey Konovalov <andreyknvl@...il.com>
Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>, Alexander Potapenko <glider@...gle.com>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Vincenzo Frascino <vincenzo.frascino@....com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Christoph Lameter <cl@...ux.com>, 
	Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>, 
	Joonsoo Kim <iamjoonsoo.kim@....com>, Vlastimil Babka <vbabka@...e.cz>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Hyeonggon Yoo <42.hyeyoo@...il.com>, 
	Marco Elver <elver@...gle.com>, kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH v5 2/2] slub: Introduce CONFIG_SLUB_RCU_DEBUG

On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@...il.com> wrote:
> On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@...gle.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.)
> >
> > Signed-off-by: Jann Horn <jannh@...gle.com>
>
> Acked-by: Andrey Konovalov <andreyknvl@...il.com>
>
> But see a comment below.
>
> > ---
> >  include/linux/kasan.h | 11 +++++---
> >  mm/Kconfig.debug      | 30 ++++++++++++++++++++
> >  mm/kasan/common.c     | 11 ++++----
> >  mm/kasan/kasan_test.c | 46 +++++++++++++++++++++++++++++++
> >  mm/slab_common.c      | 12 ++++++++
> >  mm/slub.c             | 76 +++++++++++++++++++++++++++++++++++++++++++++------
> >  6 files changed, 169 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 34cb7a25aacb..0b952e11c7a0 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -194,28 +194,30 @@ static __always_inline bool kasan_slab_pre_free(struct kmem_cache *s,
> >  {
> >         if (kasan_enabled())
> >                 return __kasan_slab_pre_free(s, object, _RET_IP_);
> >         return false;
> >  }
> >
> > -bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init);
> > +bool __kasan_slab_free(struct kmem_cache *s, void *object, bool init,
> > +                      bool after_rcu_delay);
>
> What do you think about renaming this argument to poison_rcu? I think
> it makes the intention more clear from the KASAN's point of view.

Hm - my thinking here was that the hook is an API between SLUB and
KASAN, and so the hook definition should reflect the API contract that
both SLUB and KASAN have to know - and in my head, this contract is
that the parameter says whether SLUB guarantees that an RCU delay has
happened after kmem_cache_free() was called.
In my mind, SLUB tells KASAN what is going on and gives KASAN a chance
to take ownership of the object, but doesn't instruct KASAN to do
anything specific.

And "poison" is ambiguous - in SLUB, "poison" normally refers to
overwriting object contents with a poison value, which currently
wouldn't be allowed here due to constructor slabs.

I guess another way to describe the meaning of the argument with its
current value would be something like "even though the object is an
object with RCU lifetime, the object is guaranteed to no longer be in
use". But I think the simplest way to describe the argument as
currently defined is "an RCU grace period has passed since
kmem_cache_free() was called" (which I guess I'll add to the
kasan_slab_free doc comment if we keep the current naming).

I guess I could also change the API to pass something different - like
a flag meaning "the object is guaranteed to no longer be in use".
There is already code in slab_free_hook() that computes this
expression, so we could easily pass that to KASAN and then avoid doing
the same logic in KASAN again... I think that would be the most
elegant approach?

> >  /**
> >   * kasan_slab_free - Possibly handle slab object freeing.
> >   * @object: Object to free.
>
> @poison_rcu - Whether to skip poisoning for SLAB_TYPESAFE_BY_RCU caches.
>
> And also update the reworded comment from the previous patch:
>
> This function poisons a slab object and saves a free stack trace for
> it, except for SLAB_TYPESAFE_BY_RCU caches when @poison_rcu is false.

I think that's a KASAN implementation detail, so I would prefer not
putting that in this header.

Powered by blists - more mailing lists