Re: [PATCH v4 05/11] mm, kfence: insert KFENCE hooks for SLUB

[Jann Horn <jannh@google.com>]

On Tue, Sep 29, 2020 at 3:38 PM Marco Elver <elver@google.com> wrote:
> Inserts KFENCE hooks into the SLUB allocator.
[...]
> diff --git a/mm/slub.c b/mm/slub.c
[...]
> @@ -3290,8 +3314,14 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>         c = this_cpu_ptr(s->cpu_slab);
>
>         for (i = 0; i < size; i++) {
> -               void *object = c->freelist;
> +               void *object = kfence_alloc(s, s->object_size, flags);

kfence_alloc() will invoke ->ctor() callbacks if the current slab has
them. Is it fine to invoke such callbacks from here, where we're in
the middle of a section that disables interrupts to protect against
concurrent freelist changes? If someone decides to be extra smart and
uses a kmem_cache with a ->ctor that can allocate memory from the same
kmem_cache, or something along those lines, this could lead to
corruption of the SLUB freelist. But I'm not sure whether that can
happen in practice.

Still, it might be nicer if you could code this to behave like a
fastpath miss: Update c->tid, turn interrupts back on (___slab_alloc()
will also do that if it has to call into the page allocator), then let
kfence do the actual allocation in a more normal context, then turn
interrupts back off and go on. If that's not too complicated?

Maybe Christoph Lameter has opinions on whether this is necessary...
it admittedly is fairly theoretical.

> +               if (unlikely(object)) {
> +                       p[i] = object;
> +                       continue;
> +               }
> +
> +               object = c->freelist;
>                 if (unlikely(!object)) {
>                         /*
>                          * We may have removed an object from c->freelist using