Re: [RFC PATCH 6/6] livepatch: Support replace_set in shadow variable API

On Fri, May 15, 2026 at 7:01 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Newly allocated shadow variables fail to store the `replace_set` identifier, breaking all subsequent lookups and leading to unbounded memory leaks.
> - [High] The `klp_shadow_combined_id()` function invokes undefined behavior and loses data on 32-bit architectures by shifting a 32-bit integer by 32 bits.
> - [High] Changing the shadow API `id` parameter to `unsigned int` silently truncates 64-bit `klp_state` identifiers, leading to memory leaks.
> - [Low] Modifying the `klp_shadow_*` function signatures breaks the build for in-tree callers.
> --
>
> commit 91116e31e667d0beea437b5048ab5a62099a5eed
> Author: Yafang Shao <[email protected]>
>
> livepatch: Support replace_set in shadow variable API
>
> To support more complex livepatching scenarios where multiple
> replacement sets might coexist, extend the klp_shadow API to
> include a replace_set identifier.
>
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index c9f2036dfefcf..cee35f5cae9ad 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -192,15 +192,17 @@ static inline bool klp_have_reliable_stack(void)
> >              IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
> >  }
> >
> > -void *klp_shadow_get(void *obj, unsigned long id);
> > -void *klp_shadow_alloc(void *obj, unsigned long id,
> > +void *klp_shadow_get(void *obj, unsigned int replace_set, unsigned int id);
> > +void *klp_shadow_alloc(void *obj, unsigned int replace_set, unsigned int id,
> >                      size_t size, gfp_t gfp_flags,
> >                      klp_shadow_ctor_t ctor, void *ctor_data);
>
> Does this signature change break the build for existing in-tree callers?
>
> It looks like samples/livepatch/livepatch-shadow-fix1.c and
> tools/testing/selftests/livepatch/test_modules/test_klp_shadow_vars.c
> might not have been updated to pass the new replace_set argument.

selftests will be changed accordingly once the RFC is removed.

>
> [ ... ]
>
> > diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> > index c2e724d97ddf2..35e507fae4458 100644
> > --- a/kernel/livepatch/shadow.c
> > +++ b/kernel/livepatch/shadow.c
> > @@ -59,6 +60,11 @@ struct klp_shadow {
> >       char data[];
> >  };
> >
> > +static unsigned long klp_shadow_combined_id(unsigned int set, unsigned int id)
> > +{
> > +     return ((unsigned long)set << 32) | id;
> > +}
>
> On 32-bit architectures that support livepatching (such as 32-bit PowerPC),
> unsigned long is 32 bits wide. Does shifting a 32-bit integer by 32 bits
> invoke undefined behavior here?

Ah, that is a problem on 32-bit PPC. I'll fix this in v2.

>
> Furthermore, since struct klp_shadow->id remains defined as an unsigned long,
> it cannot physically store a 64-bit combined ID on these systems. The
> replace_set value might be entirely truncated, resulting in hash collisions
> and subsequent issues when multiple sets manage the same resource.
>
> [ ... ]
>
> > @@ -126,7 +134,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
> >
> >       /* Look for <obj, id> again under the lock */
> >       spin_lock_irqsave(&klp_shadow_lock, flags);
> > -     shadow_data = klp_shadow_get(obj, id);
> > +     shadow_data = klp_shadow_get(obj, set, id);
> >       if (unlikely(shadow_data)) {
> >               /*
> >                * Shadow variable was found, throw away speculative
> > @@ -147,8 +155,8 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
> [ ... ]
> >       }
> >
> >       new_shadow->obj = obj;
> >       new_shadow->id = id;
>
> Because the id argument is now a 32-bit integer, the upper 32 bits of
> new_shadow->id will be zero. Should this be initialized using
> klp_shadow_combined_id(set, id) instead?

Right. will fix it.

>
> Lookup functions like klp_shadow_get() and klp_shadow_free() search using
> the 64-bit composite ID klp_shadow_combined_id(replace_set, id). When
> replace_set is non-zero, this lookup will always fail.
>
> This means klp_shadow_get() will erroneously return NULL, and
> klp_shadow_get_or_alloc() could duplicate variables indefinitely, leading
> to unbounded memory leaks.
>
> [ ... ]
>
> > diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> > index 43115e8e8453c..6e3d6fb92e64a 100644
> > --- a/kernel/livepatch/state.c
> > +++ b/kernel/livepatch/state.c
> > @@ -203,7 +203,8 @@ void klp_states_post_unpatch(struct klp_patch *patch)
> >                       state->callbacks.post_unpatch(patch, state);
> >
> >               if (state->is_shadow)
> > -                     klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
> > +                     klp_shadow_free_all(patch->replace_set, state->id,
> > +                                         state->callbacks.shadow_dtor);
>
> The shadow variable APIs previously accepted an unsigned long for the id,
> allowing 64-bit identifiers. The new klp_shadow_free_all() now accepts an
> unsigned int, but struct klp_state still defines id as an unsigned long.

Right, we should clearly document this API change.

>
> Will the implicit cast from unsigned long to unsigned int silently truncate
> the upper 32 bits of state->id on 64-bit platforms?
>
> If the original ID exceeded 32 bits, the core might fail to locate and free
> the correct shadow variables, causing a permanent memory leak during
> garbage collection.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6



-- 
Regards
Yafang