RE: [PATCH V1] mm, slub: avoid zeroing kmalloc redzone

[Peng Fan <peng.fan@nxp.com>]

> Subject: Re: [PATCH V1] mm, slub: avoid zeroing kmalloc redzone
> 
> On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > With commit 946fa0dbf2d8
> > ("mm/slub: extend redzone check to extra allocated kmalloc space
> than
> > requested"), setting orig_size treats the wasted space (object_size -
> > orig_size) as redzones. But (in check_object()) when orig_size is set
> > to zero, the entire object is perceived as a redzone. To a valid
> > allocated kmalloc space, when init_on_free=1, the wasted space and
> the
> > orig_size should not be cleared to 0, otherwise there will be kernel
> dump:
> >
> > [    0.000000]
> ============================================================
> =================
> > [    0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone
> overwritten
> > [    0.000000] -----------------------------------------------------------------------------
> > [    0.000000]
> > [    0.000000] 0xffff000010032858-0xffff00001003285f
> @offset=2136. First byte 0x0 instead of 0xcc
> > [    0.000000] FIX kmalloc-8: Restoring kmalloc Redzone
> 0xffff000010032858-0xffff00001003285f=0xcc
> > [    0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23
> fp=0xffff000010032a18
> flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1f
> fff)
> > [    0.000000] Object 0xffff000010032858 @offset=2136
> fp=0xffff0000100328c8
> > [    0.000000]
> > [    0.000000] Redzone  ffff000010032850: cc cc cc cc cc cc cc
> cc                          ........
> > [    0.000000] Object   ffff000010032858: cc cc cc cc cc cc cc
> cc                          ........
> > [    0.000000] Redzone  ffff000010032860: cc cc cc cc cc cc cc
> cc                          ........
> > [    0.000000] Padding  ffff0000100328b4: 00 00 00 00 00 00 00 00
> 00 00 00 00              ............
> > [    0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
> 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144
> > [    0.000000] Hardware name: NXP i.MX95 19X19 board (DT)
> > [    0.000000] Call trace:
> > [    0.000000]  dump_backtrace+0x90/0xe8
> > [    0.000000]  show_stack+0x18/0x24
> > [    0.000000]  dump_stack_lvl+0x74/0x8c
> > [    0.000000]  dump_stack+0x18/0x24
> > [    0.000000]  print_trailer+0x150/0x218
> > [    0.000000]  check_object+0xe4/0x454
> > [    0.000000]  free_to_partial_list+0x2f8/0x5ec
> >
> > To address the issue, use orig_size to clear the used area. And
> > restore the value of orig_size after clear the remaining area.
> >
> > When CONFIG_SLUB_DEBUG not defined, (get_orig_size()' directly
> returns
> > s->object_size. So when using memset to init the area, the size can
> > s->simply
> > be orig_size, as orig_size returns object_size when
> CONFIG_SLUB_DEBUG
> > not enabled. And orig_size can never be bigger than object_size.
> >
> > Fixes: 946fa0dbf2d8 ("mm/slub: extend redzone check to extra
> allocated
> > kmalloc space than requested")
> 
> Thanks for the fix! I missed to test the 'init_on_free' case back then.
> 
> Reviewed-by: Feng Tang <feng.tang@intel.com>
> 
> with one small nit below
> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > RFC->V1:
> >  Update commit log (Per Hyeonggon)
> >  Use orig_size to do memset(Per Hyeonggon)  Add get_orig_size and
> > set_orig_size when CONFIG_SLUB_DEBUG not enabled(kernel test
> robot)
> >
> >
> >
> >  mm/slub.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 94f5a4143825..a5fbeb2835b1 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1895,6 +1895,15 @@ static inline void inc_slabs_node(struct
> > kmem_cache *s, int node,  static inline void dec_slabs_node(struct
> kmem_cache *s, int node,
> >  							int objects)
> {}
> >
> > +static inline unsigned int get_orig_size(struct kmem_cache *s, void
> > +*object) {
> > +	return s->object_size;
> > +}
> > +
> > +static inline void set_orig_size(struct kmem_cache *s, void *object,
> > +				 unsigned int orig_size)
> > +{}
> 
> Current get_orig_size() and set_orig_size() are protected by
> CONFIG_SLUB_DEUG=y macro, and with this patch, they will be called
> in both ON and OFF case. Maybe we can just lift those existing
> functions out of the "#ifdef CONFIG_SLUB_DEBUG" protection?

This will build a bit more code when CONFIG_SLUB_DEBUG is
not defined. But it should be fine. I will wait to see if other people
have comments on this.

Thanks,
Peng.

> 
> Thanks,
> Feng