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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Thank you!
- Feng
