Re: [PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object when slub_debug_orig_size is on

[Andrey Konovalov <andreyknvl@gmail.com>]

On Mon, Sep 9, 2024 at 3:30 AM Feng Tang <feng.tang@intel.com> wrote:
>
> For a kmalloc object, when both kasan and slub redzone sanity check
> are enabled, they could both manipulate its data space like storing
> kasan free meta data and setting up kmalloc redzone, and may affect
> accuracy of that object's 'orig_size'.
>
> As an accurate 'orig_size' will be needed by some function like
> krealloc() soon, save kasan's free meta data in slub's metadata area
> instead of inside object when 'orig_size' is enabled.
>
> This will make it easier to maintain/understand the code. Size wise,
> when these two options are both enabled, the slub meta data space is
> already huge, and this just slightly increase the overall size.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  mm/kasan/generic.c |  5 ++++-
>  mm/slab.h          |  6 ++++++
>  mm/slub.c          | 17 -----------------
>  3 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 6310a180278b..cad376199d47 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -393,8 +393,11 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>          *    be touched after it was freed, or
>          * 2. Object has a constructor, which means it's expected to
>          *    retain its content until the next allocation.

Nit: ", or" above.

> +        * 3. It is from a kmalloc cache which enables the debug option
> +        *    to store original size.
>          */
> -       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor) {
> +       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor ||
> +            slub_debug_orig_size(cache)) {
>                 cache->kasan_info.free_meta_offset = *size;
>                 *size += sizeof(struct kasan_free_meta);
>                 goto free_meta_added;
> diff --git a/mm/slab.h b/mm/slab.h
> index 90f95bda4571..7a0e9b34ba2a 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -689,6 +689,12 @@ void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab)
>  void __check_heap_object(const void *ptr, unsigned long n,
>                          const struct slab *slab, bool to_user);
>
> +static inline bool slub_debug_orig_size(struct kmem_cache *s)
> +{
> +       return (kmem_cache_debug_flags(s, SLAB_STORE_USER) &&
> +                       (s->flags & SLAB_KMALLOC));
> +}
> +
>  #ifdef CONFIG_SLUB_DEBUG
>  void skip_orig_size_check(struct kmem_cache *s, const void *object);
>  #endif
> diff --git a/mm/slub.c b/mm/slub.c
> index 23761533329d..996a72fa6f62 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -230,12 +230,6 @@ static inline bool kmem_cache_debug(struct kmem_cache *s)
>         return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
>  }
>
> -static inline bool slub_debug_orig_size(struct kmem_cache *s)
> -{
> -       return (kmem_cache_debug_flags(s, SLAB_STORE_USER) &&
> -                       (s->flags & SLAB_KMALLOC));
> -}
> -
>  void *fixup_red_left(struct kmem_cache *s, void *p)
>  {
>         if (kmem_cache_debug_flags(s, SLAB_RED_ZONE))
> @@ -760,21 +754,10 @@ static inline void set_orig_size(struct kmem_cache *s,
>                                 void *object, unsigned int orig_size)
>  {
>         void *p = kasan_reset_tag(object);
> -       unsigned int kasan_meta_size;
>
>         if (!slub_debug_orig_size(s))
>                 return;
>
> -       /*
> -        * KASAN can save its free meta data inside of the object at offset 0.
> -        * If this meta data size is larger than 'orig_size', it will overlap
> -        * the data redzone in [orig_size+1, object_size]. Thus, we adjust
> -        * 'orig_size' to be as at least as big as KASAN's meta data.
> -        */
> -       kasan_meta_size = kasan_metadata_size(s, true);
> -       if (kasan_meta_size > orig_size)
> -               orig_size = kasan_meta_size;
> -
>         p += get_info_end(s);
>         p += sizeof(struct track) * 2;
>
> --
> 2.34.1
>

Acked-by: Andrey Konovalov <andreyknvl@gmail.com>