On Sun 2024-04-07 11:57:30, Yafang Shao wrote:
> In our production environment, upon loading a new atomic replace livepatch,
> we encountered an issue where the kernel module of the old livepatch
> remained, despite being replaced by the new one. This issue can be
> reproduced using the following steps:
>
> - Pre setup: Build two atomic replace livepatches
>
> - First step: Load the first livepatch using the insmod command
> $ insmod livepatch-test_0.ko
>
> $ ls /sys/kernel/livepatch/
> livepatch_test_0
>
> $ cat /sys/kernel/livepatch/livepatch_test_0/enabled
> 1
>
> $ lsmod | grep livepatch
> livepatch_test_0 16384 1
>
> - Second step: Load the new livepatch using the insmod command
> $ insmod livepatch-test_1.ko
>
> $ ls /sys/kernel/livepatch/
> livepatch_test_1 <<<< livepatch_test_0 was replaced
>
> $ cat /sys/kernel/livepatch/livepatch_test_1/enabled
> 1
>
> $ lsmod | grep livepatch
> livepatch_test_1 16384 1
> livepatch_test_0 16384 0 <<<< leftover
>
> Detecting which livepatch will be replaced by the new one from userspace is
> not reliable, necessitating the need for the operation to be performed
> within the kernel itself. With this improvement, executing
> `insmod livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.
>
> Following this change, the associated kernel module will be removed when
> executing `echo 0 > /sys/kernel/livepatch/${livepath}/enabled`. Therefore,
> adjustments need to be made to the selftests accordingly.
>
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -721,8 +723,12 @@ static void klp_free_patch_finish(struct klp_patch
> *patch)
> wait_for_completion(&patch->finish);
>
> /* Put the module after the last access to struct klp_patch. */
> - if (!patch->forced)
> - module_put(patch->mod);
> + if (!patch->forced) {
> + module_put(mod);
> + if (!delete)
> + return;
> + delete_module(mod);
We should check the return value and report an error.
> + }
> }
Otherwise, it looks good to me.
I am personally in favor of this change. It removes one step
which otherwise has to be called from userspace after a non trivial
delay. The transition might take seconds.
It should simplify the scripting around livepatch modules handling.
It might reduce the need to over-optimize transition times.
Best Regards,
Petr
