Re: [PATCH] mm/slub: Add check for s->flags in the alloc_tagging_slab_free_hook

[Hao Ge]

Hi Suren


Thank you for taking the time to review this patch.


On 8/16/24 05:00, Suren Baghdasaryan wrote:
> On Thu, Aug 15, 2024 at 12:21 AM Hao Ge <hao.ge@xxxxxxxxx> wrote:
> > From: Hao Ge <gehao@xxxxxxxxxx>
> >
> > When enable CONFIG_MEMCG & CONFIG_KFENCE & CONFIG_KMEMLEAK,
> > the following warning always occurs,This is because the
> > following call stack occurred:
> > mem_pool_alloc
> >      kmem_cache_alloc_noprof
> >          slab_alloc_node
> >              kfence_alloc
> >
> > Once the kfence allocation is successful,slab->obj_exts will not be empty.
> > Since in the prepare_slab_obj_exts_hook function,we perform a check for
> > s->flags & (SLAB_NO_OBJ_EXT | SLAB_NOLEAKTRACE),the alloc_tag_add function
> > will not be called as a result.Therefore,ref->ct remains NULL.
> >
> > However,when we call mem_pool_free,since obj_ext is not empty,
> > it eventually leads to the alloc_tag_sub scenario being invoked.
> > This is where the warning occurs.
> Ok, I was trying to understand why "Once the kfence allocation is
> successful,slab->obj_exts will not be empty.". alloc_slab_obj_exts()
> has to be called to create slab->obj_exts. Normally that is done in
> prepare_slab_obj_exts_hook() which has the s->flags & (SLAB_NO_OBJ_EXT
> | SLAB_NOLEAKTRACE) check and the cache that kfence_alloc() uses here
> is the object_cache (passed from mem_pool_alloc()) which has
> SLAB_NOLEAKTRACE:
> https://elixir.bootlin.com/linux/v6.11-rc3/source/mm/kmemleak.c#L453.
> Therefore, prepare_slab_obj_exts_hook() should have skipped it and did
> not create the slab->obj_exts. So, it must have been called from
> account_slab() or __memcg_slab_post_alloc_hook() to create
> slab->obj_exts for memcg accounting in slab->obj_exts.objcg. Ok, now I
> understand why you need the CONFIG_MEMCG & CONFIG_KFENCE &
> CONFIG_KMEMLEAK combination.
> Please confirm that slab->obj_exts creation is happening the way I
> described above and for those reasons and if so, please update the
> description of this patch to explain that.

It's mostly correct, but in the context of kfence, the assignment 
actually takes place within the kfence_init_pool function.

https://elixir.bootlin.com/linux/v6.11-rc3/source/mm/kfence/core.c#L606

So in version 2, I have included this explain "because it has already 
been assigned a value in kfence_init_pool."

> > So we should add corresponding checks in the alloc_tagging_slab_free_hook.
> > For __GFP_NO_OBJ_EXT case,I didn't see the specific case where it's
> > using kfence,so I won't add the corresponding check in
> > alloc_tagging_slab_free_hook for now.
> >
> > [    3.734349] ------------[ cut here ]------------
> > [    3.734807] alloc_tag was not set
> > [    3.735129] WARNING: CPU: 4 PID: 40 at ./include/linux/alloc_tag.h:130 kmem_cache_free+0x444/0x574
> > [    3.735866] Modules linked in: autofs4
> > [    3.736211] CPU: 4 UID: 0 PID: 40 Comm: ksoftirqd/4 Tainted: G        W          6.11.0-rc3-dirty #1
> > [    3.736969] Tainted: [W]=WARN
> > [    3.737258] Hardware name: QEMU KVM Virtual Machine, BIOS unknown 2/2/2022
> > [    3.737875] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    3.738501] pc : kmem_cache_free+0x444/0x574
> > [    3.738951] lr : kmem_cache_free+0x444/0x574
> > [    3.739361] sp : ffff80008357bb60
> > [    3.739693] x29: ffff80008357bb70 x28: 0000000000000000 x27: 0000000000000000
> > [    3.740338] x26: ffff80008207f000 x25: ffff000b2eb2fd60 x24: ffff0000c0005700
> > [    3.740982] x23: ffff8000804229e4 x22: ffff800082080000 x21: ffff800081756000
> > [    3.741630] x20: fffffd7ff8253360 x19: 00000000000000a8 x18: ffffffffffffffff
> > [    3.742274] x17: ffff800ab327f000 x16: ffff800083398000 x15: ffff800081756df0
> > [    3.742919] x14: 0000000000000000 x13: 205d344320202020 x12: 5b5d373038343337
> > [    3.743560] x11: ffff80008357b650 x10: 000000000000005d x9 : 00000000ffffffd0
> > [    3.744231] x8 : 7f7f7f7f7f7f7f7f x7 : ffff80008237bad0 x6 : c0000000ffff7fff
> > [    3.744907] x5 : ffff80008237ba78 x4 : ffff8000820bbad0 x3 : 0000000000000001
> > [    3.745580] x2 : 68d66547c09f7800 x1 : 68d66547c09f7800 x0 : 0000000000000000
> > [    3.746255] Call trace:
> > [    3.746530]  kmem_cache_free+0x444/0x574
> > [    3.746931]  mem_pool_free+0x44/0xf4
> > [    3.747306]  free_object_rcu+0xc8/0xdc
> > [    3.747693]  rcu_do_batch+0x234/0x8a4
> > [    3.748075]  rcu_core+0x230/0x3e4
> > [    3.748424]  rcu_core_si+0x14/0x1c
> > [    3.748780]  handle_softirqs+0x134/0x378
> > [    3.749189]  run_ksoftirqd+0x70/0x9c
> > [    3.749560]  smpboot_thread_fn+0x148/0x22c
> > [    3.749978]  kthread+0x10c/0x118
> > [    3.750323]  ret_from_fork+0x10/0x20
> > [    3.750696] ---[ end trace 0000000000000000 ]---
> >
> > Fixes: 4b8736964640 ("mm/slab: add allocation accounting into slab allocation and free paths")
> > Signed-off-by: Hao Ge <gehao@xxxxxxxxxx>
> > ---
> >   mm/slub.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index c9d8a2497fd6..1f67621ba6dc 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2116,6 +2116,9 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
> >          if (!mem_alloc_profiling_enabled())
> >                  return;
> >
> > +       if (s->flags & (SLAB_NO_OBJ_EXT | SLAB_NOLEAKTRACE))
> > +               return;
> > +
> Please add a comment before this check saying something like:
> /* slab->obj_exts might not be NULL if it was created for MEMCG accounting. */
>
> Other than that the fix seems fine to me.
> Thanks,
> Suren.
OK,I have already made the modification in version 2.
> >          obj_exts = slab_obj_exts(slab);
> >          if (!obj_exts)
> >                  return;
> > --
> > 2.25.1
Thanks

best regards

Hao