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
