Re: [RFC PATCH 1/6] livepatch: Support scoped atomic replace using replace set

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Module reference leak during forced transition.
- [High] Silent semantic reversion for zero-initialized out-of-tree livepatches.
- [High] Sysfs ABI breakage and incorrect type formatting for the `replace` attribute.
- [Medium] Build failure in livepatch selftests.
--

commit 8a05e8a3ebd9241541926ac6aba19452a820c899
Author: Yafang Shao <[email protected]>

livepatch: Support scoped atomic replace using replace set

This commit converts the boolean replace attribute to a u32 replace_set
to allow livepatches to atomically replace only existing patches within
the same set. Patches with different replace sets can now remain active
simultaneously.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
[ ... ]
> @@ -137,7 +138,7 @@ struct klp_patch {
>  	struct module *mod;
>  	struct klp_object *objs;
>  	struct klp_state *states;
> -	bool replace;
> +	unsigned int replace_set;

By removing the boolean replace flag, out-of-tree livepatches that
zero-initialize their patch structures will now default to a replace_set of 0.
Will this cause them to unintentionally replace all other zero-initialized
patches instead of stacking safely?

Additionally, does this rename break the livepatch selftests in
tools/testing/selftests/livepatch/test_modules/ (such as test_klp_state2.c),
which still explicitly set patch.replace = true?

[ ... ]
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -454,7 +454,7 @@ static ssize_t replace_show(struct kobject *kobj,
>  	struct klp_patch *patch;
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
> -	return sysfs_emit(buf, "%d\n", patch->replace);
> +	return sysfs_emit(buf, "%d\n", patch->replace_set);

Does changing the sysfs output from a boolean to a replace set ID break
compatibility for userspace tools expecting strict 0 or 1 values from the
replace attribute?

Also, since replace_set is declared as an unsigned int, will using the
signed %d format specifier cause large replace set IDs to overflow and
display as negative numbers?

[ ... ]
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
[ ... ]
> @@ -720,11 +720,11 @@ void klp_force_transition(void)
>  		klp_update_patch_state(idle_task(cpu));
>  
>  	/* Set forced flag for patches being removed. */
> -	if (klp_target_state == KLP_TRANSITION_UNPATCHED)
> +	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
>  		klp_transition_patch->forced = true;
> -	else if (klp_transition_patch->replace) {
> +	} else {
>  		klp_for_each_patch(patch) {
> -			if (patch != klp_transition_patch)
> +			if (patch->replace_set == klp_transition_patch->replace_set)
>  				patch->forced = true;
>  		}
>  	}

Could this inadvertently set forced = true on the klp_transition_patch
itself?

Since klp_transition_patch was already added to the global list during
klp_init_patch(), it will match its own replace_set here. If it is marked
as forced during a patching transition, wouldn't we leak its module reference
in klp_free_patch_finish() when it is cleanly disabled later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1