1. Patchew
  2. linux
  3. [PATCH 0/5] mm/slub: Improve data handling of krealloc() when orig_size is enabled
  4. View patch

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

Feng Tang posted 5 patches 2 months, 3 weeks ago
There is a newer version of this series
[PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object when slub_debug_orig_size is on
Posted by Feng Tang 2 months, 3 weeks ago 
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.
+	 * 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
Re: [PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object when slub_debug_orig_size is on
Posted by Andrey Konovalov 2 months, 3 weeks ago 
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>
Re: [PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object when slub_debug_orig_size is on
Posted by Feng Tang 2 months, 3 weeks ago 
On Mon, Sep 09, 2024 at 06:24:21PM +0200, Andrey Konovalov wrote:
> 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.

Aha, yes, I missed that.

Hi Vlastimil,

Could you help to change it when taking the patches, or you prefer me
to send a new version? thanks!

> 
> > +        * 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>

Thank you!

- Feng

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.