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

[Vlastimil Babka]

On 8/25/24 15:05, Feng Tang wrote:
> On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@xxxxxxx>
>> 
>> 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:

Could we make it more clear?

Since 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 a redzone. However with init_on_free=1 we clear
the full object->size, including the redzone.

Additionally we clear the object metadata, including the stored orig_size,
making it zero, which makes check_object() treat the the whole object as a
redzone.

These issues lead to the following BUG report with "slub_debug=FUZ
init_on_free=1":

>> 
>> [    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=0x1ffff)
>> [    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 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@xxxxxxxxx>

By the way Feng, have you noticed the other issue we have, with krealloc()
and redzoning? Want to look at that? Thanks.

https://lore.kernel.org/all/44fa564b-9c8f-4ac2-bce3-f6d2c99b73b7@xxxxxxx/


> with one small nit below
> 
>> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
>> ---
>> 
>> 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)
>>  https://lore.kernel.org/all/20240819064115.385086-1-peng.fan@xxxxxxxxxxx/
>> 
>>  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?

I agree. As for the "code is larger for !CONFIG_SLUB_DEBUG" concern I think
the code will be eliminated anyway, because of the

if (!slub_debug_orig_size(s))
    return;

and slub_debug_orig_size() does kmem_cache_debug_flags() which is a
compile-time false for !CONFIG_SLUB_DEBUG.

> Thanks,
> Feng