[PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Chengming Zhou]

module_put() is not called for a patch with "forced" flag. It should
block the removal of the livepatch module when the code might still
be in use after forced transition.

klp_force_transition() currently sets "forced" flag for all patches on
the list.

In fact, any patch can be safely unloaded when it passed through
the consistency model in KLP_UNPATCHED transition.

By other words, the "forced" flag must be set only for livepatches
that are being removed. In particular, set the "forced" flag:

  + only for klp_transition_patch when the transition to KLP_UNPATCHED
    state was forced.

  + all replaced patches when the transition to KLP_PATCHED state was
    forced and the patch was replacing the existing patches.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
Changes in v3:
 - rewrite more clear commit message by Petr.

Changes in v2:
 - interact nicely with the atomic replace feature noted by Miroslav.
---
 kernel/livepatch/transition.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 5683ac0d2566..7f25a5ae89f6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -641,6 +641,18 @@ void klp_force_transition(void)
 	for_each_possible_cpu(cpu)
 		klp_update_patch_state(idle_task(cpu));
 
-	klp_for_each_patch(patch)
-		patch->forced = true;
+	/*
+	 * Only need to set forced flag for the transition patch
+	 * when force transition to KLP_UNPATCHED state, but
+	 * have to set forced flag for all replaced patches
+	 * when force atomic replace transition.
+	 */
+	if (klp_target_state == KLP_UNPATCHED)
+		klp_transition_patch->forced = true;
+	else if (klp_transition_patch->replace) {
+		klp_for_each_patch(patch) {
+			if (patch != klp_transition_patch)
+				patch->forced = true;
+		}
+	}
 }
-- 
2.20.1

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Miroslav Benes]

On Sat, 12 Mar 2022, Chengming Zhou wrote:

> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
> 
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
> 
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
> 
> By other words, the "forced" flag must be set only for livepatches

s/By/In/

> that are being removed. In particular, set the "forced" flag:
> 
>   + only for klp_transition_patch when the transition to KLP_UNPATCHED
>     state was forced.
> 
>   + all replaced patches when the transition to KLP_PATCHED state was
>     forced and the patch was replacing the existing patches.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> Changes in v3:
>  - rewrite more clear commit message by Petr.
> 
> Changes in v2:
>  - interact nicely with the atomic replace feature noted by Miroslav.
> ---
>  kernel/livepatch/transition.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 5683ac0d2566..7f25a5ae89f6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -641,6 +641,18 @@ void klp_force_transition(void)
>  	for_each_possible_cpu(cpu)
>  		klp_update_patch_state(idle_task(cpu));
>  
> -	klp_for_each_patch(patch)
> -		patch->forced = true;
> +	/*
> +	 * Only need to set forced flag for the transition patch
> +	 * when force transition to KLP_UNPATCHED state, but
> +	 * have to set forced flag for all replaced patches
> +	 * when force atomic replace transition.
> +	 */

How about something like

/*
 * Set forced flag for patches being removed, which is the transition
 * patch in KLP_UNPATCHED state or all replaced patches when forcing
 * the atomic replace transition.
 */

?

> +	if (klp_target_state == KLP_UNPATCHED)
> +		klp_transition_patch->forced = true;
> +	else if (klp_transition_patch->replace) {
> +		klp_for_each_patch(patch) {
> +			if (patch != klp_transition_patch)
> +				patch->forced = true;
> +		}
> +	}

Looks good to me.

Miroslav

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Petr Mladek]

On Wed 2022-03-16 15:48:25, Miroslav Benes wrote:
> On Sat, 12 Mar 2022, Chengming Zhou wrote:
> 
> > module_put() is not called for a patch with "forced" flag. It should
> > block the removal of the livepatch module when the code might still
> > be in use after forced transition.
> > 
> > klp_force_transition() currently sets "forced" flag for all patches on
> > the list.
> > 
> > In fact, any patch can be safely unloaded when it passed through
> > the consistency model in KLP_UNPATCHED transition.
> > 
> > By other words, the "forced" flag must be set only for livepatches
> 
> s/By/In/
> 
> > that are being removed. In particular, set the "forced" flag:
> > 
> >   + only for klp_transition_patch when the transition to KLP_UNPATCHED
> >     state was forced.
> > 
> >   + all replaced patches when the transition to KLP_PATCHED state was
> >     forced and the patch was replacing the existing patches.
> > 
> > index 5683ac0d2566..7f25a5ae89f6 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -641,6 +641,18 @@ void klp_force_transition(void)
> >  	for_each_possible_cpu(cpu)
> >  		klp_update_patch_state(idle_task(cpu));
> >  
> > -	klp_for_each_patch(patch)
> > -		patch->forced = true;
> > +	/*
> > +	 * Only need to set forced flag for the transition patch
> > +	 * when force transition to KLP_UNPATCHED state, but
> > +	 * have to set forced flag for all replaced patches
> > +	 * when force atomic replace transition.
> > +	 */
> 
> How about something like
> 
> /*
>  * Set forced flag for patches being removed, which is the transition
>  * patch in KLP_UNPATCHED state or all replaced patches when forcing
>  * the atomic replace transition.
>  */

Or just the first sentence:

	/* Set forced flag for patches being removed */

The rest is visible from the code.

Either version works for me. If we agree on it then I update
the text when pushing the patch.

Best Regards,
Petr

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Miroslav Benes]

> > > + /*
> > > +	 * Only need to set forced flag for the transition patch
> > > +	 * when force transition to KLP_UNPATCHED state, but
> > > +	 * have to set forced flag for all replaced patches
> > > +	 * when force atomic replace transition.
> > > +	 */
> > 
> > How about something like
> > 
> > /*
> >  * Set forced flag for patches being removed, which is the transition
> >  * patch in KLP_UNPATCHED state or all replaced patches when forcing
> >  * the atomic replace transition.
> >  */
> 
> Or just the first sentence:
> 
> 	/* Set forced flag for patches being removed */
> 
> The rest is visible from the code.

True. This would work for me as well.

Miroslav

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Joe Lawrence]

On 3/16/22 11:03 AM, Miroslav Benes wrote:
>>>> + /*
>>>> +	 * Only need to set forced flag for the transition patch
>>>> +	 * when force transition to KLP_UNPATCHED state, but
>>>> +	 * have to set forced flag for all replaced patches
>>>> +	 * when force atomic replace transition.
>>>> +	 */
>>>
>>> How about something like
>>>
>>> /*
>>>  * Set forced flag for patches being removed, which is the transition
>>>  * patch in KLP_UNPATCHED state or all replaced patches when forcing
>>>  * the atomic replace transition.
>>>  */
>>
>> Or just the first sentence:
>>
>> 	/* Set forced flag for patches being removed */
>>
>> The rest is visible from the code.
> 
> True. This would work for me as well.
> 

Sorry for not following this one more closely as we don't use force nor
atomic replace patches (yet) ... but the code and use case seems clear
enough for the shorter comment.

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- 
Joe

Re: [External] Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Chengming Zhou]

On 2022/3/16 11:01 下午, Petr Mladek wrote:
> On Wed 2022-03-16 15:48:25, Miroslav Benes wrote:
>> On Sat, 12 Mar 2022, Chengming Zhou wrote:
>>
>>> module_put() is not called for a patch with "forced" flag. It should
>>> block the removal of the livepatch module when the code might still
>>> be in use after forced transition.
>>>
>>> klp_force_transition() currently sets "forced" flag for all patches on
>>> the list.
>>>
>>> In fact, any patch can be safely unloaded when it passed through
>>> the consistency model in KLP_UNPATCHED transition.
>>>
>>> By other words, the "forced" flag must be set only for livepatches
>>
>> s/By/In/
>>
>>> that are being removed. In particular, set the "forced" flag:
>>>
>>>   + only for klp_transition_patch when the transition to KLP_UNPATCHED
>>>     state was forced.
>>>
>>>   + all replaced patches when the transition to KLP_PATCHED state was
>>>     forced and the patch was replacing the existing patches.
>>>
>>> index 5683ac0d2566..7f25a5ae89f6 100644
>>> --- a/kernel/livepatch/transition.c
>>> +++ b/kernel/livepatch/transition.c
>>> @@ -641,6 +641,18 @@ void klp_force_transition(void)
>>>  	for_each_possible_cpu(cpu)
>>>  		klp_update_patch_state(idle_task(cpu));
>>>  
>>> -	klp_for_each_patch(patch)
>>> -		patch->forced = true;
>>> +	/*
>>> +	 * Only need to set forced flag for the transition patch
>>> +	 * when force transition to KLP_UNPATCHED state, but
>>> +	 * have to set forced flag for all replaced patches
>>> +	 * when force atomic replace transition.
>>> +	 */
>>
>> How about something like
>>
>> /*
>>  * Set forced flag for patches being removed, which is the transition
>>  * patch in KLP_UNPATCHED state or all replaced patches when forcing
>>  * the atomic replace transition.
>>  */
> 
> Or just the first sentence:
> 
> 	/* Set forced flag for patches being removed */
> 
> The rest is visible from the code.
> 
> Either version works for me. If we agree on it then I update
> the text when pushing the patch.

Ok, this works for me too ;-)

Thanks.

> 
> Best Regards,
> Petr

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Petr Mladek]

On Sat 2022-03-12 23:22:20, Chengming Zhou wrote:
> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
> 
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
> 
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
> 
> By other words, the "forced" flag must be set only for livepatches
> that are being removed. In particular, set the "forced" flag:
> 
>   + only for klp_transition_patch when the transition to KLP_UNPATCHED
>     state was forced.
> 
>   + all replaced patches when the transition to KLP_PATCHED state was
>     forced and the patch was replacing the existing patches.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

Looks reasonable, passes livepatch tests:

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

Just let me repeat. The "force" flags must be used carefully because
it breaks the consistency model.

Best Regards,
Petr

Re: [PATCH v3] livepatch: Don't block removal of patches that are safe to unload

[Petr Mladek]

On Sat 2022-03-12 23:22:20, Chengming Zhou wrote:
> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
> 
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
> 
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
> 
> By other words, the "forced" flag must be set only for livepatches
> that are being removed. In particular, set the "forced" flag:
> 
>   + only for klp_transition_patch when the transition to KLP_UNPATCHED
>     state was forced.
> 
>   + all replaced patches when the transition to KLP_PATCHED state was
>     forced and the patch was replacing the existing patches.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

The patch has been committed, with the proposed wording changes,
into livepatching/livepatching.git, branch for-5.18/fixes,
see
https://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git/commit/?h=for-5.18/fixes&id=2957308343fa7c621df9f342fab88cb970b8d5f3

Best Regards,
Petr