Re: [PATCH] mm/slab: Initialise random_kmalloc_seed after initcalls

[Huacai Chen <chenhuacai@kernel.org>]

Hi, Harry,

On Wed, Feb 12, 2025 at 11:39 PM Harry (Hyeonggon) Yoo
<42.hyeyoo@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 11:17 PM Huacai Chen <chenhuacai@loongson.cn> wrote:
> >
> > Hibernation assumes the memory layout after resume be the same as that
> > before sleep, but CONFIG_RANDOM_KMALLOC_CACHES breaks this assumption.
>
> [Let's also Cc SLAB ALLOCATOR folks in MAINTAINERS file]
>
> Could you please elaborate what do you mean by
> hibernation assumes 'the memory layout' after resume be the same as that
> before sleep?
>
> I don't understand how updating random_kmalloc_seed breaks resuming from
> hibernation. Changing random_kmalloc_seed affects which kmalloc caches
> newly allocated objects are from, but it should not affect the objects that are
> already allocated (before hibernation).
When resuming, the booting kernel should switch to the target kernel,
if the address of switch code (from the booting kernel) is the
effective data of the target kernel, then the switch code may be
overwritten.

For LoongArch there is an additional problem: the regular kernel
function uses absolute address to call exception handlers, this means
the code calls to exception handlers should at the same address for
booting kernel and target kernel.

>
> > At least on LoongArch and ARM64 we observed failures of resuming from
> > hibernation (on LoongArch non-boot CPUs fail to bringup, on ARM64 some
> > devices are unusable).
>
> Did you have any chance to reproduce it on x86_64?
I haven't reproduce on x86_64, but I have heard that x86_32 has problems.

>
> > software_resume_initcall(), the function which resume the target kernel
> > is a initcall function. So, move the random_kmalloc_seed initialisation
> > after all initcalls.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 3c6152940584290668 ("Randomized slab caches for kmalloc()")
> > Reported-by: Yuli Wang <wangyuli@uniontech.com>
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >
> >  init/main.c      | 3 +++
> >  mm/slab_common.c | 3 ---
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/init/main.c b/init/main.c
> > index 2a1757826397..1362957bdbe4 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1458,6 +1458,9 @@ static int __ref kernel_init(void *unused)
> >         /* need to finish all async __init code before freeing the memory */
> >         async_synchronize_full();
> >
> > +#ifdef CONFIG_RANDOM_KMALLOC_CACHES
> > +       random_kmalloc_seed = get_random_u64();
> > +#endif
>
> It doesn’t seem appropriate to put slab code in kernel_init.
>
> Additionally, it introduces a dependency that the code must be executed
> after all late_initcalls, which sounds like introducing yet another
> type of initcall.
What about introducing a function to initialize kmalloc seed in
slab_common.c, and then call it at kernel_init()? I don't have a
better solution than this.

>
> >         system_state = SYSTEM_FREEING_INITMEM;
> >         kprobe_free_init_mem();
> >         ftrace_free_init_mem();
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 4030907b6b7d..23e324aee218 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -971,9 +971,6 @@ void __init create_kmalloc_caches(void)
> >                 for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++)
> >                         new_kmalloc_cache(i, type);
> >         }
> > -#ifdef CONFIG_RANDOM_KMALLOC_CACHES
> > -       random_kmalloc_seed = get_random_u64();
> > -#endif
>
> I have no idea how hibernation and resume work, but let me ask here:
> Can we simply skip or defer updating random_kmalloc_seed when the system is
> resuming from hibernation? (probably system_state represents this?)
Do you mean something like below? It does work (it is my original
solution), but this patch is simpler.

diff --git a/include/linux/slab.h b/include/linux/slab.h
index b35e2db7eb0e..42fb91650b13 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -614,14 +614,20 @@ static __always_inline enum kmalloc_cache_type
kmalloc_type(gfp_t flags, unsigne
         * The most common case is KMALLOC_NORMAL, so test for it
         * with a single branch for all the relevant flags.
         */
-       if (likely((flags & KMALLOC_NOT_NORMAL_BITS) == 0))
+       if (likely((flags & KMALLOC_NOT_NORMAL_BITS) == 0)) {
 #ifdef CONFIG_RANDOM_KMALLOC_CACHES
+               unsigned long random_seed = 0;
+
+               if (system_state > SYSTEM_SCHEDULING)
+                       random_seed = random_kmalloc_seed;
+
                /* RANDOM_KMALLOC_CACHES_NR (=15) copies + the KMALLOC_NORMAL */
-               return KMALLOC_RANDOM_START + hash_64(caller ^
random_kmalloc_seed,
+               return KMALLOC_RANDOM_START + hash_64(caller ^ random_seed,

ilog2(RANDOM_KMALLOC_CACHES_NR + 1));
 #else
                return KMALLOC_NORMAL;
 #endif
+       }


Huacai

>
> >         /* Kmalloc array is now usable */
> >         slab_state = UP;
>
> --
> Harry