[v6,3/4] mm: kasan: Add free_meta size info in struct kasan_cache

Commit Message

Feng Tang Sept. 13, 2022, 6:54 a.m. UTC

When kasan is enabled for slab/slub, it may save kasan' free_meta
data in the former part of slab object data area in slab object's
free path, which works fine.

There is ongoing effort to extend slub's debug function which will
redzone the latter part of kmalloc object area, and when both of
the debug are enabled, there is possible conflict, especially when
the kmalloc object has small size, as caught by 0Day bot [1]

For better information for slab/slub, add free_meta's data size
into 'struct kasan_cache', so that its users can take right action
to avoid data conflict.

[1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
Reported-by: kernel test robot <[e-mail]>
Signed-off-by: Feng Tang <[e-mail]>
Acked-by: Dmitry Vyukov <[e-mail]>
---
 include/linux/kasan.h | 2 ++
 mm/kasan/common.c     | 2 ++
 2 files changed, 4 insertions(+)

Comments

Andrey Konovalov Sept. 20, 2022, 7:20 p.m. UTC | #1

On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <[e-mail]> wrote:
>
> When kasan is enabled for slab/slub, it may save kasan' free_meta
> data in the former part of slab object data area in slab object's
> free path, which works fine.
>
> There is ongoing effort to extend slub's debug function which will
> redzone the latter part of kmalloc object area, and when both of
> the debug are enabled, there is possible conflict, especially when
> the kmalloc object has small size, as caught by 0Day bot [1]
>
> For better information for slab/slub, add free_meta's data size
> into 'struct kasan_cache', so that its users can take right action
> to avoid data conflict.
>
> [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
> Reported-by: kernel test robot <[e-mail]>
> Signed-off-by: Feng Tang <[e-mail]>
> Acked-by: Dmitry Vyukov <[e-mail]>
> ---
>  include/linux/kasan.h | 2 ++
>  mm/kasan/common.c     | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index b092277bf48d..49af9513e8ed 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void)
>  struct kasan_cache {
>         int alloc_meta_offset;
>         int free_meta_offset;
> +       /* size of free_meta data saved in object's data area */
> +       int free_meta_size;
>         bool is_kmalloc;
>  };
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 69f583855c8b..0cb867e92524 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -201,6 +201,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>                         cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
>                         *size = ok_size;
>                 }
> +       } else {
> +               cache->kasan_info.free_meta_size = sizeof(struct kasan_free_meta);

Hi Feng,

I just realized that we already have a function that exposes a similar
functionality: kasan_metadata_size. However, this function returns the
size of metadata that is stored in the redzone.

I think, instead of adding free_meta_size, a better approach would be to:

1. Rename kasan_metadata_size to kasan_metadata_size_in_redzone (or
something like that).
2. Add kasan_metadata_size_in_object with appropriate implementation
and use that in your patches.

This allows avoiding exposing KASAN-internal details such as what kind
of fields the kasan_cache struct has to the common code.

Sorry for nor realizing this straight away.

(Note that there's an upcoming patch that fixes a bug in
kasan_metadata_size' implementation [1].)

Thanks!

[1] https://lore.kernel.org/linux-mm/c7[e-mail]/



>         }
>
>         /* Calculate size with optimal redzone. */
> --
> 2.34.1
>

Feng Tang Sept. 21, 2022, 12:02 p.m. UTC | #2

Hi Andrey,

On Wed, Sep 21, 2022 at 03:20:58AM +0800, Andrey Konovalov wrote:
> On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <[e-mail]> wrote:
> >
> > When kasan is enabled for slab/slub, it may save kasan' free_meta
> > data in the former part of slab object data area in slab object's
> > free path, which works fine.
> >
> > There is ongoing effort to extend slub's debug function which will
> > redzone the latter part of kmalloc object area, and when both of
> > the debug are enabled, there is possible conflict, especially when
> > the kmalloc object has small size, as caught by 0Day bot [1]
> >
> > For better information for slab/slub, add free_meta's data size
> > into 'struct kasan_cache', so that its users can take right action
> > to avoid data conflict.
> >
> > [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
> > Reported-by: kernel test robot <[e-mail]>
> > Signed-off-by: Feng Tang <[e-mail]>
> > Acked-by: Dmitry Vyukov <[e-mail]>
> > ---
> >  include/linux/kasan.h | 2 ++
> >  mm/kasan/common.c     | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index b092277bf48d..49af9513e8ed 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void)
> >  struct kasan_cache {
> >         int alloc_meta_offset;
> >         int free_meta_offset;
> > +       /* size of free_meta data saved in object's data area */
> > +       int free_meta_size;
> >         bool is_kmalloc;
> >  };
> >
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 69f583855c8b..0cb867e92524 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -201,6 +201,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
> >                         cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
> >                         *size = ok_size;
> >                 }
> > +       } else {
> > +               cache->kasan_info.free_meta_size = sizeof(struct kasan_free_meta);
> 
> Hi Feng,
> 
> I just realized that we already have a function that exposes a similar
> functionality: kasan_metadata_size. However, this function returns the
> size of metadata that is stored in the redzone.
> 
> I think, instead of adding free_meta_size, a better approach would be to:
> 
> 1. Rename kasan_metadata_size to kasan_metadata_size_in_redzone (or
> something like that).
> 2. Add kasan_metadata_size_in_object with appropriate implementation
> and use that in your patches.
> 
> This allows avoiding exposing KASAN-internal details such as what kind
> of fields the kasan_cache struct has to the common code.

Agree, it's better not touch the internal fields in slub code.

How about the following patch, it merge the 2 functions with one flag
indicating in meta data or object. (I'm fine with 2 separate functions) 


diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b092277bf48d..0ad05a34e708 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -150,11 +150,12 @@ static __always_inline void kasan_cache_create_kmalloc(struct kmem_cache *cache)
 		__kasan_cache_create_kmalloc(cache);
 }
 
-size_t __kasan_metadata_size(struct kmem_cache *cache);
-static __always_inline size_t kasan_metadata_size(struct kmem_cache *cache)
+size_t __kasan_meta_size(struct kmem_cache *cache, bool in_slab_object);
+static __always_inline size_t kasan_meta_size(struct kmem_cache *cache,
+							bool in_slab_object)
 {
 	if (kasan_enabled())
-		return __kasan_metadata_size(cache);
+		return  __kasan_meta_size(cache, in_slab_object);
 	return 0;
 }
 
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 69f583855c8b..2a8710461ebb 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -218,14 +218,21 @@ void __kasan_cache_create_kmalloc(struct kmem_cache *cache)
 	cache->kasan_info.is_kmalloc = true;
 }
 
-size_t __kasan_metadata_size(struct kmem_cache *cache)
+size_t __kasan_meta_size(struct kmem_cache *cache, bool in_slab_object)
 {
 	if (!kasan_stack_collection_enabled())
 		return 0;
-	return (cache->kasan_info.alloc_meta_offset ?
-		sizeof(struct kasan_alloc_meta) : 0) +
-		(cache->kasan_info.free_meta_offset ?
-		sizeof(struct kasan_free_meta) : 0);
+
+	if (in_slab_object)
+		return (cache->kasan_info.alloc_meta_offset == 0 ?
+			sizeof(struct kasan_alloc_meta) : 0) +
+			(cache->kasan_info.free_meta_offset ?
+			sizeof(struct kasan_free_meta) : 0);
+	else
+		return (cache->kasan_info.alloc_meta_offset == 0 ?
+			sizeof(struct kasan_alloc_meta) : 0) +
+			(cache->kasan_info.free_meta_offset ?
+			sizeof(struct kasan_free_meta) : 0);
 }
 
 struct kasan_alloc_meta *kasan_get_alloc_meta(struct kmem_cache *cache,

> Sorry for nor realizing this straight away.
> 
> (Note that there's an upcoming patch that fixes a bug in
> kasan_metadata_size' implementation [1].)
 
Thanks for the note, will check this when making the formal patch. 

- Feng

> Thanks!
> 
> [1] https://lore.kernel.org/linux-mm/c7[e-mail]/

Andrey Konovalov Sept. 24, 2022, 6:05 p.m. UTC | #3

On Wed, Sep 21, 2022 at 2:03 PM Feng Tang <[e-mail]> wrote:
>
> Agree, it's better not touch the internal fields in slub code.
>
> How about the following patch, it merge the 2 functions with one flag
> indicating in meta data or object. (I'm fine with 2 separate functions)

The overall approach sounds good. See some comments below.

> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index b092277bf48d..0ad05a34e708 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -150,11 +150,12 @@ static __always_inline void kasan_cache_create_kmalloc(struct kmem_cache *cache)
>                 __kasan_cache_create_kmalloc(cache);
>  }
>
> -size_t __kasan_metadata_size(struct kmem_cache *cache);
> -static __always_inline size_t kasan_metadata_size(struct kmem_cache *cache)
> +size_t __kasan_meta_size(struct kmem_cache *cache, bool in_slab_object);
> +static __always_inline size_t kasan_meta_size(struct kmem_cache *cache,
> +                                                       bool in_slab_object)

I would keep the name as kasan_metadata_size as it's more clear to
external users but rename in_slab_object to in_object to make the
declaration shorter.

>  {
>         if (kasan_enabled())
> -               return __kasan_metadata_size(cache);
> +               return  __kasan_meta_size(cache, in_slab_object);
>         return 0;
>  }
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 69f583855c8b..2a8710461ebb 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -218,14 +218,21 @@ void __kasan_cache_create_kmalloc(struct kmem_cache *cache)
>         cache->kasan_info.is_kmalloc = true;
>  }
>
> -size_t __kasan_metadata_size(struct kmem_cache *cache)
> +size_t __kasan_meta_size(struct kmem_cache *cache, bool in_slab_object)
>  {
>         if (!kasan_stack_collection_enabled())
>                 return 0;
> -       return (cache->kasan_info.alloc_meta_offset ?
> -               sizeof(struct kasan_alloc_meta) : 0) +
> -               (cache->kasan_info.free_meta_offset ?
> -               sizeof(struct kasan_free_meta) : 0);
> +
> +       if (in_slab_object)
> +               return (cache->kasan_info.alloc_meta_offset == 0 ?
> +                       sizeof(struct kasan_alloc_meta) : 0) +
> +                       (cache->kasan_info.free_meta_offset ?
> +                       sizeof(struct kasan_free_meta) : 0);
> +       else
> +               return (cache->kasan_info.alloc_meta_offset == 0 ?
> +                       sizeof(struct kasan_alloc_meta) : 0) +
> +                       (cache->kasan_info.free_meta_offset ?
> +                       sizeof(struct kasan_free_meta) : 0);

Something weird here: both if and else cases are the same.

The change also needs to be rebased onto [1].

Thanks!

[1] https://lore.kernel.org/linux-mm/c7[e-mail]/

Feng Tang Sept. 25, 2022, 11:26 a.m. UTC | #4

On Sun, Sep 25, 2022 at 02:05:04AM +0800, Andrey Konovalov wrote:
> On Wed, Sep 21, 2022 at 2:03 PM Feng Tang <[e-mail]> wrote:
> >
> > Agree, it's better not touch the internal fields in slub code.
> >
> > How about the following patch, it merge the 2 functions with one flag
> > indicating in meta data or object. (I'm fine with 2 separate functions)
> 
> The overall approach sounds good. See some comments below.
> 
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index b092277bf48d..0ad05a34e708 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -150,11 +150,12 @@ static __always_inline void kasan_cache_create_kmalloc(struct kmem_cache *cache)
> >                 __kasan_cache_create_kmalloc(cache);
> >  }
> >
> > -size_t __kasan_metadata_size(struct kmem_cache *cache);
> > -static __always_inline size_t kasan_metadata_size(struct kmem_cache *cache)
> > +size_t __kasan_meta_size(struct kmem_cache *cache, bool in_slab_object);
> > +static __always_inline size_t kasan_meta_size(struct kmem_cache *cache,
> > +                                                       bool in_slab_object)
> 
> I would keep the name as kasan_metadata_size as it's more clear to
> external users but rename in_slab_object to in_object to make the
> declaration shorter.

Make sense to me, will do.

[...]

> > +       if (in_slab_object)
> > +               return (cache->kasan_info.alloc_meta_offset == 0 ?
> > +                       sizeof(struct kasan_alloc_meta) : 0) +
> > +                       (cache->kasan_info.free_meta_offset ?
> > +                       sizeof(struct kasan_free_meta) : 0);
> > +       else
> > +               return (cache->kasan_info.alloc_meta_offset == 0 ?
> > +                       sizeof(struct kasan_alloc_meta) : 0) +
> > +                       (cache->kasan_info.free_meta_offset ?
> > +                       sizeof(struct kasan_free_meta) : 0);
> 
> Something weird here: both if and else cases are the same.
 
Yes, will fix it. 

> The change also needs to be rebased onto [1].
> 
> Thanks!
> 
> [1] https://lore.kernel.org/linux-mm/c7[e-mail]/

I noticed this has been merged to -mm tree's 'mm-everything' branch,
so following is the patch againt that. Thanks! 

One thing I'm not very sure is, to check 'in-object' kasan's meta
size, I didn't check 'alloc_meta_offset', as from the code reading
the alloc_meta is never put inside slab object data area.


Thanks,
Feng

---8<---

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index d811b3d7d2a1..96c9d56e5510 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
 
 #ifdef CONFIG_KASAN_GENERIC
 
-size_t kasan_metadata_size(struct kmem_cache *cache);
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object);
 slab_flags_t kasan_never_merge(void);
 void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 			slab_flags_t *flags);
@@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr);
 #else /* CONFIG_KASAN_GENERIC */
 
 /* Tag-based KASAN modes do not use per-object metadata. */
-static inline size_t kasan_metadata_size(struct kmem_cache *cache)
+static inline size_t kasan_metadata_size(struct kmem_cache *cache,
+						bool in_object)
 {
 	return 0;
 }
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index d8b5590f9484..5a806f9b9466 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
 		__memset(alloc_meta, 0, sizeof(*alloc_meta));
 }
 
-size_t kasan_metadata_size(struct kmem_cache *cache)
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
 {
+	struct kasan_cache *info = &cache->kasan_info ;
+
 	if (!kasan_requires_meta())
 		return 0;
-	return (cache->kasan_info.alloc_meta_offset ?
-		sizeof(struct kasan_alloc_meta) : 0) +
-		((cache->kasan_info.free_meta_offset &&
-		  cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ?
-		 sizeof(struct kasan_free_meta) : 0);
+
+	if (in_object)
+		return (info->free_meta_offset ?
+			0 : sizeof(struct kasan_free_meta));
+	else
+		return (info->alloc_meta_offset ?
+			sizeof(struct kasan_alloc_meta) : 0) +
+			((info->free_meta_offset &&
+			info->free_meta_offset != KASAN_NO_FREE_META) ?
+			sizeof(struct kasan_free_meta) : 0);
 }
 
 static void __kasan_record_aux_stack(void *addr, bool can_alloc)
diff --git a/mm/slub.c b/mm/slub.c
index ce8310e131b3..a75c21a0da8b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -887,7 +887,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
 	if (s->flags & SLAB_STORE_USER)
 		off += 2 * sizeof(struct track);
 
-	off += kasan_metadata_size(s);
+	off += kasan_metadata_size(s, false);
 
 	if (off != size_from_object(s))
 		/* Beginning of the filler is the free pointer */
@@ -1042,7 +1042,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
 		/* We also have user information there */
 		off += 2 * sizeof(struct track);
 
-	off += kasan_metadata_size(s);
+	off += kasan_metadata_size(s, false);
 
 	if (size_from_object(s) == off)
 		return 1;

Andrey Konovalov Sept. 25, 2022, 4:31 p.m. UTC | #5

On Sun, Sep 25, 2022 at 1:27 PM Feng Tang <[e-mail]> wrote:
>
> > [1] https://lore.kernel.org/linux-mm/c7[e-mail]/
>
> I noticed this has been merged to -mm tree's 'mm-everything' branch,
> so following is the patch againt that. Thanks!
>
> One thing I'm not very sure is, to check 'in-object' kasan's meta
> size, I didn't check 'alloc_meta_offset', as from the code reading
> the alloc_meta is never put inside slab object data area.

Yes, this is correct.

> Thanks,
> Feng
>
> ---8<---
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index d811b3d7d2a1..96c9d56e5510 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
>
>  #ifdef CONFIG_KASAN_GENERIC
>
> -size_t kasan_metadata_size(struct kmem_cache *cache);
> +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object);
>  slab_flags_t kasan_never_merge(void);
>  void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>                         slab_flags_t *flags);
> @@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr);
>  #else /* CONFIG_KASAN_GENERIC */
>
>  /* Tag-based KASAN modes do not use per-object metadata. */
> -static inline size_t kasan_metadata_size(struct kmem_cache *cache)
> +static inline size_t kasan_metadata_size(struct kmem_cache *cache,
> +                                               bool in_object)
>  {
>         return 0;
>  }
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index d8b5590f9484..5a806f9b9466 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
>                 __memset(alloc_meta, 0, sizeof(*alloc_meta));
>  }
>
> -size_t kasan_metadata_size(struct kmem_cache *cache)
> +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
>  {
> +       struct kasan_cache *info = &cache->kasan_info ;
> +
>         if (!kasan_requires_meta())
>                 return 0;
> -       return (cache->kasan_info.alloc_meta_offset ?
> -               sizeof(struct kasan_alloc_meta) : 0) +
> -               ((cache->kasan_info.free_meta_offset &&
> -                 cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ?
> -                sizeof(struct kasan_free_meta) : 0);
> +
> +       if (in_object)
> +               return (info->free_meta_offset ?
> +                       0 : sizeof(struct kasan_free_meta));
> +       else
> +               return (info->alloc_meta_offset ?
> +                       sizeof(struct kasan_alloc_meta) : 0) +
> +                       ((info->free_meta_offset &&
> +                       info->free_meta_offset != KASAN_NO_FREE_META) ?
> +                       sizeof(struct kasan_free_meta) : 0);
>  }
>
>  static void __kasan_record_aux_stack(void *addr, bool can_alloc)
> diff --git a/mm/slub.c b/mm/slub.c
> index ce8310e131b3..a75c21a0da8b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -887,7 +887,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
>         if (s->flags & SLAB_STORE_USER)
>                 off += 2 * sizeof(struct track);
>
> -       off += kasan_metadata_size(s);
> +       off += kasan_metadata_size(s, false);
>
>         if (off != size_from_object(s))
>                 /* Beginning of the filler is the free pointer */
> @@ -1042,7 +1042,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
>                 /* We also have user information there */
>                 off += 2 * sizeof(struct track);
>
> -       off += kasan_metadata_size(s);
> +       off += kasan_metadata_size(s, false);
>
>         if (size_from_object(s) == off)
>                 return 1;

Reviewed-by: Andrey Konovalov <[e-mail]>

Thanks!

Feng Tang Sept. 27, 2022, 3:03 a.m. UTC | #6

On Mon, Sep 26, 2022 at 12:31:37AM +0800, Andrey Konovalov wrote:
> On Sun, Sep 25, 2022 at 1:27 PM Feng Tang <[e-mail]> wrote:
> >
> > > [1] https://lore.kernel.org/linux-mm/c7[e-mail]/
> >
> > I noticed this has been merged to -mm tree's 'mm-everything' branch,
> > so following is the patch againt that. Thanks!
> >
> > One thing I'm not very sure is, to check 'in-object' kasan's meta
> > size, I didn't check 'alloc_meta_offset', as from the code reading
> > the alloc_meta is never put inside slab object data area.
> 
> Yes, this is correct.
> 
> > @@ -1042,7 +1042,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
> >                 /* We also have user information there */
> >                 off += 2 * sizeof(struct track);
> >
> > -       off += kasan_metadata_size(s);
> > +       off += kasan_metadata_size(s, false);
> >
> >         if (size_from_object(s) == off)
> >                 return 1;
> 
> Reviewed-by: Andrey Konovalov <[e-mail]>
 
Thank you!

I made a formal patch, which is based on your latest kasan patchset
in -mm tree

Thanks,
Feng

---
From ba4cfd81b86c3339523b467451baa5e87ca1c9f8 Mon Sep 17 00:00:00 2001
From: Feng Tang <[e-mail]>
Date: Sun, 25 Sep 2022 15:37:31 +0800
Subject: [PATCH] mm: kasan: Extend kasan_metadata_size() to also cover
 in-object size

When kasan is enabled for slab/slub, it may save kasan' free_meta
data in the former part of slab object data area in slab object's
free path, which works fine.

There is ongoing effort to extend slub's debug function which will
redzone the latter part of kmalloc object area, and when both of
the debug are enabled, there is possible conflict, especially when
the kmalloc object has small size, as caught by 0Day bot [1].

To solve it, slub code needs to know the in-object kasan's meta
data size. Currently, there is existing kasan_metadata_size()
which returns the kasan's metadata size inside slub's metadata
area, so extend it to also cover the in-object meta size by
adding a boolean flag 'in_object'.

There is no functional change to existing code logic.

[1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
Reported-by: kernel test robot <[e-mail]>
Signed-off-by: Feng Tang <[e-mail]>
Suggested-by: Andrey Konovalov <[e-mail]>
Reviewed-by: Andrey Konovalov <[e-mail]>
---
 include/linux/kasan.h |  5 +++--
 mm/kasan/generic.c    | 19 +++++++++++++------
 mm/slub.c             |  4 ++--
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index d811b3d7d2a1..96c9d56e5510 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
 
 #ifdef CONFIG_KASAN_GENERIC
 
-size_t kasan_metadata_size(struct kmem_cache *cache);
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object);
 slab_flags_t kasan_never_merge(void);
 void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 			slab_flags_t *flags);
@@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr);
 #else /* CONFIG_KASAN_GENERIC */
 
 /* Tag-based KASAN modes do not use per-object metadata. */
-static inline size_t kasan_metadata_size(struct kmem_cache *cache)
+static inline size_t kasan_metadata_size(struct kmem_cache *cache,
+						bool in_object)
 {
 	return 0;
 }
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index d8b5590f9484..b076f597a378 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
 		__memset(alloc_meta, 0, sizeof(*alloc_meta));
 }
 
-size_t kasan_metadata_size(struct kmem_cache *cache)
+size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
 {
+	struct kasan_cache *info = &cache->kasan_info;
+
 	if (!kasan_requires_meta())
 		return 0;
-	return (cache->kasan_info.alloc_meta_offset ?
-		sizeof(struct kasan_alloc_meta) : 0) +
-		((cache->kasan_info.free_meta_offset &&
-		  cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ?
-		 sizeof(struct kasan_free_meta) : 0);
+
+	if (in_object)
+		return (info->free_meta_offset ?
+			0 : sizeof(struct kasan_free_meta));
+	else
+		return (info->alloc_meta_offset ?
+			sizeof(struct kasan_alloc_meta) : 0) +
+			((info->free_meta_offset &&
+			info->free_meta_offset != KASAN_NO_FREE_META) ?
+			sizeof(struct kasan_free_meta) : 0);
 }
 
 static void __kasan_record_aux_stack(void *addr, bool can_alloc)
diff --git a/mm/slub.c b/mm/slub.c
index ce8310e131b3..a75c21a0da8b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -887,7 +887,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
 	if (s->flags & SLAB_STORE_USER)
 		off += 2 * sizeof(struct track);
 
-	off += kasan_metadata_size(s);
+	off += kasan_metadata_size(s, false);
 
 	if (off != size_from_object(s))
 		/* Beginning of the filler is the free pointer */
@@ -1042,7 +1042,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
 		/* We also have user information there */
 		off += 2 * sizeof(struct track);
 
-	off += kasan_metadata_size(s);
+	off += kasan_metadata_size(s, false);
 
 	if (size_from_object(s) == off)
 		return 1;

Patch

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b092277bf48d..49af9513e8ed 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -100,6 +100,8 @@  static inline bool kasan_has_integrated_init(void)
 struct kasan_cache {
 	int alloc_meta_offset;
 	int free_meta_offset;
+	/* size of free_meta data saved in object's data area */
+	int free_meta_size;
 	bool is_kmalloc;
 };
 
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 69f583855c8b..0cb867e92524 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -201,6 +201,8 @@  void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 			cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
 			*size = ok_size;
 		}
+	} else {
+		cache->kasan_info.free_meta_size = sizeof(struct kasan_free_meta);
 	}
 
 	/* Calculate size with optimal redzone. */