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. [ ... ] > 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? 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? 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. 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
