Re: [PATCH v3] lib/alloc_tag: do not acquire non-existent lock in alloc_tag_top_users()

[Suren Baghdasaryan <surenb@google.com>]

On Tue, Jun 24, 2025 at 7:28 AM David Wang <00107082@163.com> wrote:
>
>
> At 2025-06-24 22:13:49, "Harry Yoo" <harry.yoo@oracle.com> wrote:
> >On Tue, Jun 24, 2025 at 10:00:48PM +0800, David Wang wrote:
> >>
> >> At 2025-06-24 21:50:02, "Harry Yoo" <harry.yoo@oracle.com> wrote:
> >> >On Tue, Jun 24, 2025 at 09:25:58PM +0800, David Wang wrote:
> >> >>
> >> >> At 2025-06-24 15:25:13, "Harry Yoo" <harry.yoo@oracle.com> wrote:
> >> >> >alloc_tag_top_users() attempts to lock alloc_tag_cttype->mod_lock
> >> >> >even when the alloc_tag_cttype is not allocated because:
> >> >> >
> >> >> >  1) alloc tagging is disabled because mem profiling is disabled
> >> >> >     (!alloc_tag_cttype)
> >> >> >  2) alloc tagging is enabled, but not yet initialized (!alloc_tag_cttype)
> >> >> >  3) alloc tagging is enabled, but failed initialization
> >> >> >     (!alloc_tag_cttype or IS_ERR(alloc_tag_cttype))
> >> >> >
> >> >> >In all cases, alloc_tag_cttype is not allocated, and therefore
> >> >> >alloc_tag_top_users() should not attempt to acquire the semaphore.
> >> >> >
> >> >> >This leads to a crash on memory allocation failure by attempting to
> >> >> >acquire a non-existent semaphore:
> >> >> >
> >> >> >  Oops: general protection fault, probably for non-canonical address 0xdffffc000000001b: 0000 [#3] SMP KASAN NOPTI
> >> >> >  KASAN: null-ptr-deref in range [0x00000000000000d8-0x00000000000000df]
> >> >> >  CPU: 2 UID: 0 PID: 1 Comm: systemd Tainted: G      D             6.16.0-rc2 #1 VOLUNTARY
> >> >> >  Tainted: [D]=DIE
> >> >> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> >> >> >  RIP: 0010:down_read_trylock+0xaa/0x3b0
> >> >> >  Code: d0 7c 08 84 d2 0f 85 a0 02 00 00 8b 0d df 31 dd 04 85 c9 75 29 48 b8 00 00 00 00 00 fc ff df 48 8d 6b 68 48 89 ea 48 c1 ea 03 <80> 3c 02 00 0f 85 88 02 00 00 48 3b 5b 68 0f 85 53 01 00 00 65 ff
> >> >> >  RSP: 0000:ffff8881002ce9b8 EFLAGS: 00010016
> >> >> >  RAX: dffffc0000000000 RBX: 0000000000000070 RCX: 0000000000000000
> >> >> >  RDX: 000000000000001b RSI: 000000000000000a RDI: 0000000000000070
> >> >> >  RBP: 00000000000000d8 R08: 0000000000000001 R09: ffffed107dde49d1
> >> >> >  R10: ffff8883eef24e8b R11: ffff8881002cec20 R12: 1ffff11020059d37
> >> >> >  R13: 00000000003fff7b R14: ffff8881002cec20 R15: dffffc0000000000
> >> >> >  FS:  00007f963f21d940(0000) GS:ffff888458ca6000(0000) knlGS:0000000000000000
> >> >> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> >> >  CR2: 00007f963f5edf71 CR3: 000000010672c000 CR4: 0000000000350ef0
> >> >> >  Call Trace:
> >> >> >   <TASK>
> >> >> >   codetag_trylock_module_list+0xd/0x20
> >> >> >   alloc_tag_top_users+0x369/0x4b0
> >> >> >   __show_mem+0x1cd/0x6e0
> >> >> >   warn_alloc+0x2b1/0x390
> >> >> >   __alloc_frozen_pages_noprof+0x12b9/0x21a0
> >> >> >   alloc_pages_mpol+0x135/0x3e0
> >> >> >   alloc_slab_page+0x82/0xe0
> >> >> >   new_slab+0x212/0x240
> >> >> >   ___slab_alloc+0x82a/0xe00
> >> >> >   </TASK>
> >> >> >
> >> >> >As David Wang points out, this issue became easier to trigger after commit
> >> >> >780138b12381 ("alloc_tag: check mem_profiling_support in alloc_tag_init").
> >> >> >
> >> >> >Before the commit, the issue occurred only when it failed to allocate
> >> >> >and initialize alloc_tag_cttype or if a memory allocation fails before
> >> >> >alloc_tag_init() is called. After the commit, it can be easily triggered
> >> >> >when memory profiling is compiled but disabled at boot.
> >> >> >
> >> >> >To properly determine whether alloc_tag_init() has been called and
> >> >> >its data structures initialized, verify that alloc_tag_cttype is a valid
> >> >> >pointer before acquiring the semaphore. If the variable is NULL or an error
> >> >> >value, it has not been properly initialized. In such a case, just skip
> >> >> >and do not attempt to acquire the semaphore.
> >> >> >
> >> >> >Reported-by: kernel test robot <oliver.sang@intel.com>
> >> >> >Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-lkp/202506181351.bba867dd-lkp@intel.com__;!!ACWV5N9M2RV99hQ!MADvGKtvTvlLXNxlrJ4BdOSnbsJlyrSroPUGJ3JQHs_IF-gxxqfQ89OTZ21aN96DbmjG9qH3Wi1MlgtiSA$
> >> >> >Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-lkp/202506131711.5b41931c-lkp@intel.com__;!!ACWV5N9M2RV99hQ!MADvGKtvTvlLXNxlrJ4BdOSnbsJlyrSroPUGJ3JQHs_IF-gxxqfQ89OTZ21aN96DbmjG9qH3Wi0o2OoynA$
> >> >> >Fixes: 780138b12381 ("alloc_tag: check mem_profiling_support in alloc_tag_init")
> >> >> >Fixes: 1438d349d16b ("lib: add memory allocations report in show_mem()")
> >> >> >Cc: stable@vger.kernel.org
> >> >> >Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> >> >> >---
> >> >> >
> >> >> >@Suren: I did not add another pr_warn() because every error path in
> >> >> >alloc_tag_init() already has pr_err().
> >> >> >
> >> >> >v2 -> v3:
> >> >> >- Added another Closes: tag (David)
> >> >> >- Moved the condition into a standalone if block for better readability
> >> >> >  (Suren)
> >> >> >- Typo fix (Suren)
> >> >> >
> >> >> > lib/alloc_tag.c | 3 +++
> >> >> > 1 file changed, 3 insertions(+)
> >> >> >
> >> >> >diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> >> >> >index 41ccfb035b7b..e9b33848700a 100644
> >> >> >--- a/lib/alloc_tag.c
> >> >> >+++ b/lib/alloc_tag.c
> >> >> >@@ -127,6 +127,9 @@ size_t alloc_tag_top_users(struct codetag_bytes *tags, size_t count, bool can_sl
> >> >> >         struct codetag_bytes n;
> >> >> >         unsigned int i, nr = 0;
> >> >> >
> >> >> >+        if (IS_ERR_OR_NULL(alloc_tag_cttype))
> >> >> >+                return 0;
> >> >>
> >> >> What about mem_profiling_support set to 0 after alloc_tag_init, in this case:
> >> >> alloc_tag_cttype != NULL && mem_profiling_support==0
> >> >>
> >> >> I kind of think alloc_tag_top_users should return 0 in this case....and  both mem_profiling_support and alloc_tag_cttype should be checked....
> >> >
> >> >After commit 780138b12381, alloc_tag_cttype is not allocated if
> >> >!mem_profiling_support. (And that's  why this bug showed up)
> >>
> >> There is a sysctl(/proc/sys/vm/mem_profiling) which can override mem_profiling_support and set it to 0, after alloc_tag_init with mem_profiling_support=1

Wait, /proc/sys/vm/mem_profiling is changing mem_alloc_profiling_key,
not mem_profiling_support. Am I missing something?

> >
> >Ok. Maybe it shouldn't report memory allocation information that is
> >collected before mem profiling was disabled. (I'm not sure why it disabling
> >at runtime is allowed, though)
> >
> >That's a good thing to have, but I think that's a behavioral change in
> >mem profiling, irrelevant to this bug and not a -stable thing.
> >
> >Maybe as a follow-up patch?
>
> Only a little more changes needed, I was suggesting:
>
> @@ -134,6 +122,14 @@ size_t alloc_tag_top_users(struct codetag_bytes *tags, size_t count, bool can_sl
>         struct codetag_bytes n;
>         unsigned int i, nr = 0;
>
> +       if (!mem_profiling_support)
> +               return 0;

David is right that with /proc/sys/vm/mem_profiling memory profiling
can be turned off at runtime but the above condition should be:

if (!mem_alloc_profiling_enabled())
        return 0;


> +
> +       if (IS_ERR_OR_NULL(alloc_tag_cttype)) {
> +               pr_warn("alloctag module is not ready yet.\n");

I don't think spitting out this warning on every show_mem() is useful.
If alloc_tag_cttype is invalid because codetag_register_type() failed
then we already print an error here:
https://elixir.bootlin.com/linux/v6.16-rc3/source/lib/alloc_tag.c#L829,
so user has the logs to track this down.
If show_mem() is called so early that alloc_tag_init() hasn't been
called yet then missing allocation tag information would not be
surprising I think, considering it's early boot. I don't think it's
worth detecting and reporting such a state.

> +               return 0;
> +       }
> +
>         if (can_sleep)
>
>
>
> David