Re: [PATCH] mm/slub: return 0 when object pointer is NULL
From: Hyeonggon Yoo
Date: Mon Sep 12 2022 - 03:48:09 EST
On Mon, Sep 12, 2022 at 03:29:30PM +0800, Ben Luo wrote:
> Hello Hyeonggon,
>
> Thanks for replying :)
>
> 在 2022/9/12 15:18, Hyeonggon Yoo 写道:
> > On Mon, Sep 12, 2022 at 01:59:39PM +0800, Ben Luo wrote:
> > > NULL is definitly not a valid address
> > >
> > > Signed-off-by: Ben Luo <luoben@xxxxxxxxxxxxxxxxx>
> > > ---
> > > mm/slub.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index 862dbd9..50fad18 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -674,7 +674,7 @@ static inline int check_valid_pointer(struct kmem_cache *s,
> > > void *base;
> > > if (!object)
> > > - return 1;
> > > + return 0;
> > > base = slab_address(slab);
> > > object = kasan_reset_tag(object);
> > > --
> > > 1.8.3.1
> > >
> > Hello Ben.
> >
> > The return value is used to check if the @object has valid pointer
> > in @slab. (used for debugging) the return value is 0 if valid, 1 if invalid.
> >
> > It does not return a pointer. So changing it to 0 because 1 is invalid
> > address does not make sense.
>
> I know the meaning of this return value, but I think this function was
> expected by returning 0 if invalid ,1 if valid
>
Ah, right. Sorry for my silly mistake. I misread your patch and the code :D
Hmm then the question is: Why does check_valid_pointer() allow passing NULL?
So I just compiled and ran it with debugging enabled.
And I got:
[ 0.195507] =============================================================================
[ 0.195507] BUG ftrace_event_field (Tainted: G B ): Freechain corrupt
[ 0.195507] -----------------------------------------------------------------------------
[ 0.195507]
[ 0.195507] Slab 0xffffea000400d380 objects=28 used=28 fp=0x0000000000000000 flags=0x17ffffc0000200(slab|node=0|zone=2|lastcpu)
[ 0.195508] Object 0xffff88810034eab8 @offset=2744 fp=0x0000000000000000
[ 0.195508]
[ 0.195508] Redzone ffff88810034eab0: bb bb bb bb bb bb bb bb ........
[ 0.195509] Object ffff88810034eab8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 0.195509] Object ffff88810034eac8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 0.195510] Object ffff88810034ead8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
[ 0.195510] Redzone ffff88810034eae8: bb bb bb bb bb bb bb bb ........
[ 0.195510] Padding ffff88810034eb38: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
[ 0.195511] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G B 6.0.0-rc3+ #1765
[ 0.195511] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
[ 0.195511] Call Trace:
[ 0.195512] <TASK>
[ 0.195512] dump_stack_lvl+0x45/0x5e
[ 0.195513] object_err+0x30/0x44
[ 0.195513] deactivate_slab.cold+0x15/0x2c
[ 0.195514] ? __trace_early_add_events+0x3e/0x60
[ 0.195515] ? trace_event_init+0xaf/0x2cd
[ 0.195516] ? start_kernel+0x705/0x97d
[ 0.195517] ? check_object+0x138/0x250
[ 0.195518] ? init_object+0x6f/0x90
[ 0.195518] ? trace_define_field+0x50/0xe0
[ 0.195519] ___slab_alloc+0x4a0/0x6c0
[ 0.195520] ? trace_define_field+0x50/0xe0
[ 0.195521] ? trace_create_new_event+0x31/0xf0
[ 0.195522] ? trace_define_field+0x50/0xe0
[ 0.195523] kmem_cache_alloc+0x2a1/0x2f0
[ 0.195524] trace_define_field+0x50/0xe0
[ 0.195525] event_define_fields+0x8a/0xc0
[ 0.195527] __trace_early_add_events+0x3e/0x60
[ 0.195528] trace_event_init+0xaf/0x2cd
[ 0.195529] start_kernel+0x705/0x97d
[ 0.195529] ? x86_family+0x5/0x30
[ 0.195530] secondary_startup_64_no_verify+0xe5/0xeb
[ 0.195532] </TASK>
[ 0.195532] FIX ftrace_event_field: Isolate corrupted freechain
The call chain is:
deactivate_slab()
-> freelist_corrupted()
check_valid_pointer()
Hmm check_valid_pointer() is used to to check if the free pointer is valid.
and as NULL is used to mark that there is no next object, I think
check_valid_pointer() shuold return 1 if object is NULL.
Thanks!
> Check this original code:
>
> if (object < base || object >= base + slab->objects * s->size ||
> (object - base) % s->size) {
> return 0;
> }
>
> Object not in range of [base, base+length) is an invalid slab address, and
> it will return 0
>
>
> --
>
> Thanks,
>
> Ben
>
> >
--
Thanks,
Hyeonggon