Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux