Re: [PATCH mm] slub, kasan: improve interaction of KASAN and slub_debug poisoning

[Feng Tang]

Hi Andrey,

On Thu, Nov 23, 2023 at 05:12:08PM +0100, Andrey Konovalov wrote:
> On Thu, Nov 23, 2023 at 7:35 AM Feng Tang <feng.tang@xxxxxxxxx> wrote:
> >
> 
> Hi Feng,
> 
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -870,20 +870,20 @@ 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;
> > >
> > > -#ifdef CONFIG_KASAN_GENERIC
> > >       /*
> > > -      * KASAN could save its free meta data in object's data area at
> > > -      * offset 0, if the size is larger than 'orig_size', it will
> > > -      * overlap the data redzone in [orig_size+1, object_size], and
> > > -      * the check should be skipped.
> > > +      * 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.
> > >        */
> > > -     if (kasan_metadata_size(s, true) > orig_size)
> > > -             orig_size = s->object_size;
> > > -#endif
> > > +     kasan_meta_size = kasan_metadata_size(s, true);
> > > +     if (kasan_meta_size > orig_size)
> > > +             orig_size = kasan_meta_size;
> >
> > 'orig_size' is to save the orignal request size for kmalloc object,
> > and its main purpose is to detect the memory wastage of kmalloc
> > objects, see commit 6edf2576a6cc "mm/slub: enable debugging memory
> > wasting of kmalloc"
> >
> > Setting "orig_size = s->object_size" was to skip the wastage check
> > and the redzone sanity check for this 'wasted space'.
> 
> Yes, I get that.
> 
> The point of my change was to allow slub_debug detecting overwrites in
> the [kasan_meta_size, object_size) range when KASAN stores its free
> meta in the [0, kasan_meta_size) range. If orig_size is set to
> object_size, writes to that area will not be detected. I also thought
> that using kasan_meta_size instead of object_size for orig_size might
> give the reader better understanding of the memory layout.
> 
> > So it's better not to set 'kasan_meta_size' to orig_size.
> 
> I don't have a strong preference here: slub_debug and KASAN are not
> really meant to be used together anyway. So if you prefer, I can
> revert this change and keep using object_size as before.

Thanks for the explanation! I got your point now. I'm fine with either
way, as this change can help to enforce the redzone check for all
kmalloc objects, while can make some debug wastage info less accurate. 

Thanks,
Feng

> 
> > And from the below code, IIUC, the orig_size is not used in fixing
> > the boot problem found by Hyeonggon?
> 
> No, this is a just a partially-related clean up. It just seemed
> natural to include it into the fix, as it also touches the code around
> a kasan_metadata_size call.
> 
> Thanks!