Re: [PATCH -next] mm/slub: fix a slab missed to be freed problem

[Hyeonggon Yoo]

On Fri, Sep 30, 2022 at 04:43:23PM +0200, Vlastimil Babka wrote:
> On 9/30/22 13:51, Feng Tang wrote:
> > On Fri, Sep 30, 2022 at 07:25:54PM +0800, Vlastimil Babka wrote:
> >> 
> >> On 9/30/22 12:07, Feng Tang wrote:
> >> > When enable kasan and kfence's in-kernel kunit test with slub_debug on,
> >> > it caught a problem (in linux-next tree):
> >> > 
> >> >  ------------[ cut here ]------------
> >> >  kmem_cache_destroy test: Slab cache still has objects when called from test_exit+0x1a/0x30
> >> >  WARNING: CPU: 3 PID: 240 at mm/slab_common.c:492 kmem_cache_destroy+0x16c/0x170
> >> 
> >> Assuming the warning was preceded by some kunit test failures?
> >> I don't see how leaving more empty slabs on free list than needed would
> >> cause this warning, the shutdown should just drop the empty slab.
> > 
> > The previous code only call remove_partial() to dequeue the slab from
> > partial list, and miss to call discard_slab() for it.
> > 
> > From the debug dump, the n->nr_partils stays at 5, while n->nr_slabs
> > keeps increasing. And during shutdown, the free_partial() only free
> > the 5 slabs on partial list, and n->nr_slabs still has a big numbers
> > of orphan slabs
> 
> Thanks, finally I get the exact cause now. I've added the more detailed
> explanation to commit log and the result is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=for-6.1/slub_validation_locking&id=b731e3575f7a45a46512708f9fdf953b40c46a53
>

Very nice finding Feng, thanks!

Yeah, there are some cases where first and second check do not agree,
leading unfreed slabs.

the latest version looks good to me,
Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>

Nit:
- discard_slab() is not what's actually called, but I get what you mean anyway...
- s/Reoganize/Reorganize/g

> >> >  Modules linked in:
> >> >  CPU: 3 PID: 240 Comm: kunit_try_catch Tainted: G    B            N 6.0.0-rc7-next-20220929 #52
> >> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> >> >  RIP: 0010:kmem_cache_destroy+0x16c/0x170
> >> >  Code: 41 5c 41 5d e9 a5 04 0b 00 c3 cc cc cc cc 48 8b 55 60 48 8b 4c 24 20 48 c7 c6 40 37 d2 82 48 c7 c7 e8 a0 33 83 e8 4e d7 14 01 <0f> 0b eb a7 41 56 41 89 d6 41 55 49 89 f5 41 54 49 89 fc 55 48 89
> >> >  RSP: 0000:ffff88800775fea0 EFLAGS: 00010282
> >> >  RAX: 0000000000000000 RBX: ffffffff83bdec48 RCX: 0000000000000000
> >> >  RDX: 0000000000000001 RSI: 1ffff11000eebf9e RDI: ffffed1000eebfc6
> >> >  RBP: ffff88804362fa00 R08: ffffffff81182e58 R09: ffff88800775fbdf
> >> >  R10: ffffed1000eebf7b R11: 0000000000000001 R12: 000000008c800d00
> >> >  R13: ffff888005e78040 R14: 0000000000000000 R15: ffff888005cdfad0
> >> >  FS:  0000000000000000(0000) GS:ffff88807ed00000(0000) knlGS:0000000000000000
> >> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> >  CR2: 0000000000000000 CR3: 000000000360e001 CR4: 0000000000370ee0
> >> >  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> >  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> >  Call Trace:
> >> >   <TASK>
> >> >   test_exit+0x1a/0x30
> >> >   kunit_try_run_case+0xad/0xc0
> >> >   kunit_generic_run_threadfn_adapter+0x26/0x50
> >> >   kthread+0x17b/0x1b0
> >> > 
> >> > It was biscted to commit c7323a5ad078 ("mm/slub: restrict sysfs
> >> > validation to debug caches and make it safe")
> >> > 
> >> > The problem is inside free_debug_processing(), in one path, the slab
> >> > on partial list is missed to be freed when partial list is full.
> >> > 
> >> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
> >> > ---
> >> > 
> >> > Hi reviewers,
> >> > 
> >> > Sorry for the late reporting, but it's curious that this problem didn't
> >> > show up in my earlier test (which caught some other problems).
> >> 
> >> I think we can reuse the slab_free and don't need a new bool?
> > 
> > Yes, much simpler!
> > 
> > Thanks,
> > Feng
> > 
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 5c3c31a154ba..a63953f649ed 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -2886,22 +2886,25 @@ static noinline void free_debug_processing(
> >>                 set_freepointer(s, tail, prior);
> >>                 slab->freelist = head;
> >>  
> >> -               /* Do we need to remove the slab from full or partial list? */
> >> +               /*
> >> +                * If the slab is empty, and node's partial list is full,
> >> +                * it should be discarded anyway no matter it's on full or
> >> +                * partial list.
> >> +                */
> >> +               if (slab->inuse == 0 && n->nr_partial >= s->min_partial)
> >> +                       slab_free = slab;
> >> +
> >>                 if (!prior) {
> >> +                       /* was on full list */
> >>                         remove_full(s, n, slab);
> >> -               } else if (slab->inuse == 0 &&
> >> -                          n->nr_partial >= s->min_partial) {
> >> +                       if (!slab_free) {
> >> +                               add_partial(n, slab, DEACTIVATE_TO_TAIL);
> >> +                               stat(s, FREE_ADD_PARTIAL);
> >> +                       }
> >> +               } else if (slab_free) {
> >>                         remove_partial(n, slab);
> >>                         stat(s, FREE_REMOVE_PARTIAL);
> >>                 }
> >> -
> >> -               /* Do we need to discard the slab or add to partial list? */
> >> -               if (slab->inuse == 0 && n->nr_partial >= s->min_partial) {
> >> -                       slab_free = slab;
> >> -               } else if (!prior) {
> >> -                       add_partial(n, slab, DEACTIVATE_TO_TAIL);
> >> -                       stat(s, FREE_ADD_PARTIAL);
> >> -               }
> >>         }
> >>  
> >>         if (slab_free) {
> 

-- 
Thanks,
Hyeonggon