On 8/8/24 20:30, Jann Horn 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reviewed-by: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> Acked-by: Marco Elver <elver@xxxxxxxxxx>
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
Acked-by: Vlastimil Babka <vbabka@xxxxxxx> [slab]
Just some very minor suggestions:
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -582,12 +582,24 @@ void kmem_cache_destroy(struct kmem_cache *s)
> rcu_set = s->flags & SLAB_TYPESAFE_BY_RCU;
>
> s->refcount--;
> if (s->refcount)
> goto out_unlock;
>
> + if (IS_ENABLED(CONFIG_SLUB_RCU_DEBUG) &&
> + (s->flags & SLAB_TYPESAFE_BY_RCU)) {
> + /*
> + * Under CONFIG_SLUB_RCU_DEBUG, when objects in a
> + * SLAB_TYPESAFE_BY_RCU slab are freed, SLUB will internally
> + * defer their freeing with call_rcu().
> + * Wait for such call_rcu() invocations here before actually
> + * destroying the cache.
> + */
> + rcu_barrier();
If we wanted to be really nice and not do rcu_barrier() with the mutex held
(but it's a debugging config so who cares, probably), we could do it before
taking the mutex. It won't be even done unnecessarily as
SLAB_TYPESAFE_BY_RCU cannot merge so refcount should always go from 1 to 0
for there.
> + }
> +
> err = shutdown_cache(s);
> WARN(err, "%s %s: Slab cache still has objects when called from %pS",
> __func__, s->name, (void *)_RET_IP_);
> out_unlock:
> mutex_unlock(&slab_mutex);
> cpus_read_unlock();
> diff --git a/mm/slub.c b/mm/slub.c
> index 0c98b6a2124f..eb68f4a69f59 100644
<snip>
> +#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);
> + void *object = delayed_free->object;
> + struct slab *slab = virt_to_slab(object);
> + struct kmem_cache *s;
> +
> + kfree(delayed_free);
> +
> + if (WARN_ON(is_kfence_address(object)))
> + return;
> +
> + /* find the object and the cache again */
> + if (WARN_ON(!slab))
> + return;
> + s = slab->slab_cache;
> + if (WARN_ON(!(s->flags & SLAB_TYPESAFE_BY_RCU)))
> + return;
> +
> + /* resume freeing */
> + if (!slab_free_hook(s, object, slab_want_init_on_free(s), true))
> + return;
> + do_slab_free(s, slab, object, object, 1, _THIS_IP_);
Nit: at this point we could just do the more standard pattern
if (slab_free_hook())
fo_slab_free()
