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

[sashiko-bot@kernel.org]

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 <laoar.shao@gmail.com>

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/20260513143321.26185-1-laoar.shao@gmail.com?part=6