Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Sep 2022 16:47:46 +0900
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Ben Luo <luoben@linux.alibaba.com>
Cc:     cl@linux.com, penberg@kernel.org, rientjes@google.com,
        iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, vbabka@suse.cz,
        roman.gushchin@linux.dev, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/slub: return 0 when object pointer is NULL
Message-ID: <Yx7kIjx+MCLe4So8@hyeyoo>
References: <1662962379-16174-1-git-send-email-luoben@linux.alibaba.com>
 <Yx7dKxwxlNmCr9Ai@hyeyoo>
 <133458f0-23c9-9421-abb8-a50d5551b524@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <133458f0-23c9-9421-abb8-a50d5551b524@linux.alibaba.com>
Precedence: bulk

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@linux.alibaba.com>
> > > ---
> > >   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