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

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

[Chengming Zhou]

module_put() is currently never called for a patch with forced flag, to block
the removal of that patch module that might still be in use after a forced
transition.

But klp_force_transition() will set all patches on the list to be forced, since
commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
has removed stack ordering of the livepatches, it will cause all other patches can't
be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.

In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
transition. It can still be unloaded safely as long as it has passed through
the consistency model in KLP_UNPATCHED transition.

But the exception is when force transition of an atomic replace patch, we
have to set all previous patches to forced, or they will be removed at
the end of klp_try_complete_transition().

This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
case, and keep the old behavior when in atomic replace case.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
v2: interact nicely with the atomic replace feature noted by Miroslav.
---
 kernel/livepatch/transition.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 5683ac0d2566..34ffb8c014ed 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -641,6 +641,10 @@ 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;
+	if (klp_target_state == KLP_UNPATCHED)
+		klp_transition_patch->forced = true;
+	else if (klp_transition_patch->replace) {
+		klp_for_each_patch(patch)
+			patch->forced = true;
+	}
 }
-- 
2.20.1

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

[Petr Mladek]

On Thu 2022-03-03 18:54:46, Chengming Zhou wrote:
> module_put() is currently never called for a patch with forced flag, to block
> the removal of that patch module that might still be in use after a forced
> transition.
> 
> But klp_force_transition() will set all patches on the list to be forced, since
> commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
> has removed stack ordering of the livepatches, it will cause all other patches can't
> be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.
> 
> In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
> transition. It can still be unloaded safely as long as it has passed through
> the consistency model in KLP_UNPATCHED transition.

It really looks safe. klp_check_stack_func() makes sure that @new_func
is not on the stack when klp_target_state == KLP_UNPATCHED. As a
result, the system should not be using code from the livepatch module
when KLP_UNPATCHED transition cleanly finished.


> But the exception is when force transition of an atomic replace patch, we
> have to set all previous patches to forced, or they will be removed at
> the end of klp_try_complete_transition().
> 
> This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
> case, and keep the old behavior when in atomic replace case.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v2: interact nicely with the atomic replace feature noted by Miroslav.
> ---
>  kernel/livepatch/transition.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 5683ac0d2566..34ffb8c014ed 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -641,6 +641,10 @@ 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;
> +	if (klp_target_state == KLP_UNPATCHED)
> +		klp_transition_patch->forced = true;
> +	else if (klp_transition_patch->replace) {
> +		klp_for_each_patch(patch)
> +			patch->forced = true;

This works only because there is should be only one patch when
klp_target_state == KLP_UNPATCHED and
klp_transition_patch->forced == true.
But it is a bit tricky. I would do it the other way:

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

It looks more sane. And it makes it more clear
that the special handling of KLP_UNPATCHED transition
is done only when the atomic replace is not used.

Otherwise, I do not see any real problem with the patch.

Best Regards,
Petr

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

[Miroslav Benes]

On Tue, 8 Mar 2022, Petr Mladek wrote:

> On Thu 2022-03-03 18:54:46, Chengming Zhou wrote:
> > module_put() is currently never called for a patch with forced flag, to block
> > the removal of that patch module that might still be in use after a forced
> > transition.
> > 
> > But klp_force_transition() will set all patches on the list to be forced, since
> > commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
> > has removed stack ordering of the livepatches, it will cause all other patches can't
> > be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.
> > 
> > In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
> > transition. It can still be unloaded safely as long as it has passed through
> > the consistency model in KLP_UNPATCHED transition.
> 
> It really looks safe. klp_check_stack_func() makes sure that @new_func
> is not on the stack when klp_target_state == KLP_UNPATCHED. As a
> result, the system should not be using code from the livepatch module
> when KLP_UNPATCHED transition cleanly finished.
> 
> 
> > But the exception is when force transition of an atomic replace patch, we
> > have to set all previous patches to forced, or they will be removed at
> > the end of klp_try_complete_transition().
> > 
> > This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
> > case, and keep the old behavior when in atomic replace case.
> > 
> > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> > ---
> > v2: interact nicely with the atomic replace feature noted by Miroslav.
> > ---
> >  kernel/livepatch/transition.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 5683ac0d2566..34ffb8c014ed 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -641,6 +641,10 @@ 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;
> > +	if (klp_target_state == KLP_UNPATCHED)
> > +		klp_transition_patch->forced = true;
> > +	else if (klp_transition_patch->replace) {
> > +		klp_for_each_patch(patch)
> > +			patch->forced = true;
> 
> This works only because there is should be only one patch when
> klp_target_state == KLP_UNPATCHED and
> klp_transition_patch->forced == true.

I probably misunderstand, but the above is not generally true, is it? I 
mean, if the transition patch is forced during its disablement, it does 
not say anything about the amount of enabled patches.

> But it is a bit tricky. I would do it the other way:
> 
> 	if (klp_transition_patch->replace) {
> 		klp_for_each_patch(patch)
> 			patch->forced = true;
> 	} else if (klp_target_state == KLP_UNPATCHED) {
> 		klp_transition_patch->forced = true;
> 	}
> 
> It looks more sane. And it makes it more clear
> that the special handling of KLP_UNPATCHED transition
> is done only when the atomic replace is not used.

But it is not the same. ->replace being true only comes into play when a 
patch is enabled. If it is disabled, then it behaves like any other patch.

So, if there is ->replace patch enabled (and it is the only patch present) 
and then more !->replace patches are loaded and then if ->replace patch is 
disabled and forced, your proposal would give a different result than what 
Chengming submitted, because in your case all the other patches will get 
->forced set to true, while it is not the case in the original. It would 
be an unnecessary restriction if I am not missing something.

However, I may got lost somewhere along the way.

Regards
Miroslav

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

[Chengming Zhou]

Hi,

On 2022/3/9 1:49 上午, Miroslav Benes wrote:
> On Tue, 8 Mar 2022, Petr Mladek wrote:
> 
>> On Thu 2022-03-03 18:54:46, Chengming Zhou wrote:
>>> module_put() is currently never called for a patch with forced flag, to block
>>> the removal of that patch module that might still be in use after a forced
>>> transition.
>>>
>>> But klp_force_transition() will set all patches on the list to be forced, since
>>> commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
>>> has removed stack ordering of the livepatches, it will cause all other patches can't
>>> be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.
>>>
>>> In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
>>> transition. It can still be unloaded safely as long as it has passed through
>>> the consistency model in KLP_UNPATCHED transition.
>>
>> It really looks safe. klp_check_stack_func() makes sure that @new_func
>> is not on the stack when klp_target_state == KLP_UNPATCHED. As a
>> result, the system should not be using code from the livepatch module
>> when KLP_UNPATCHED transition cleanly finished.
>>
>>
>>> But the exception is when force transition of an atomic replace patch, we
>>> have to set all previous patches to forced, or they will be removed at
>>> the end of klp_try_complete_transition().
>>>
>>> This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
>>> case, and keep the old behavior when in atomic replace case.
>>>
>>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>>> ---
>>> v2: interact nicely with the atomic replace feature noted by Miroslav.
>>> ---
>>>  kernel/livepatch/transition.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
>>> index 5683ac0d2566..34ffb8c014ed 100644
>>> --- a/kernel/livepatch/transition.c
>>> +++ b/kernel/livepatch/transition.c
>>> @@ -641,6 +641,10 @@ 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;
>>> +	if (klp_target_state == KLP_UNPATCHED)
>>> +		klp_transition_patch->forced = true;
>>> +	else if (klp_transition_patch->replace) {
>>> +		klp_for_each_patch(patch)
>>> +			patch->forced = true;
>>
>> This works only because there is should be only one patch when
>> klp_target_state == KLP_UNPATCHED and
>> klp_transition_patch->forced == true.
> 
> I probably misunderstand, but the above is not generally true, is it? I 
> mean, if the transition patch is forced during its disablement, it does 
> not say anything about the amount of enabled patches.
> 
>> But it is a bit tricky. I would do it the other way:
>>
>> 	if (klp_transition_patch->replace) {
>> 		klp_for_each_patch(patch)
>> 			patch->forced = true;
>> 	} else if (klp_target_state == KLP_UNPATCHED) {
>> 		klp_transition_patch->forced = true;
>> 	}
>>
>> It looks more sane. And it makes it more clear
>> that the special handling of KLP_UNPATCHED transition
>> is done only when the atomic replace is not used.
> 
> But it is not the same. ->replace being true only comes into play when a 
> patch is enabled. If it is disabled, then it behaves like any other patch.
> 
> So, if there is ->replace patch enabled (and it is the only patch present) 
> and then more !->replace patches are loaded and then if ->replace patch is 
> disabled and forced, your proposal would give a different result than what 
> Chengming submitted, because in your case all the other patches will get 
> ->forced set to true, while it is not the case in the original. It would 
> be an unnecessary restriction if I am not missing something.

At first glance, I thought both way is right. But after looking at the case
you mentioned above, they are not the same indeed. The original patch
treat ->replace and not ->replace patches the same in KLP_UNPATCHED transition,
and only set all patches to forced in the atomic replace transition.

Thanks.

> 
> However, I may got lost somewhere along the way.
> 
> Regards
> Miroslav

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

[Petr Mladek]

On Thu 2022-03-10 20:57:54, Chengming Zhou wrote:
> Hi,
> 
> On 2022/3/9 1:49 上午, Miroslav Benes wrote:
> > On Tue, 8 Mar 2022, Petr Mladek wrote:
> > 
> >> On Thu 2022-03-03 18:54:46, Chengming Zhou wrote:
> >>> module_put() is currently never called for a patch with forced flag, to block
> >>> the removal of that patch module that might still be in use after a forced
> >>> transition.
> >>>
> >>> But klp_force_transition() will set all patches on the list to be forced, since
> >>> commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
> >>> has removed stack ordering of the livepatches, it will cause all other patches can't
> >>> be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.
> >>>
> >>> In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
> >>> transition. It can still be unloaded safely as long as it has passed through
> >>> the consistency model in KLP_UNPATCHED transition.
> >>
> >> It really looks safe. klp_check_stack_func() makes sure that @new_func
> >> is not on the stack when klp_target_state == KLP_UNPATCHED. As a
> >> result, the system should not be using code from the livepatch module
> >> when KLP_UNPATCHED transition cleanly finished.
> >>
> >>
> >>> But the exception is when force transition of an atomic replace patch, we
> >>> have to set all previous patches to forced, or they will be removed at
> >>> the end of klp_try_complete_transition().
> >>>
> >>> This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
> >>> case, and keep the old behavior when in atomic replace case.
> >>>
> >>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> >>> ---
> >>> v2: interact nicely with the atomic replace feature noted by Miroslav.
> >>> ---
> >>>  kernel/livepatch/transition.c | 8 ++++++--
> >>>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> >>> index 5683ac0d2566..34ffb8c014ed 100644
> >>> --- a/kernel/livepatch/transition.c
> >>> +++ b/kernel/livepatch/transition.c
> >>> @@ -641,6 +641,10 @@ 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;
> >>> +	if (klp_target_state == KLP_UNPATCHED)
> >>> +		klp_transition_patch->forced = true;
> >>> +	else if (klp_transition_patch->replace) {
> >>> +		klp_for_each_patch(patch)
> >>> +			patch->forced = true;
> >>
> >> This works only because there is should be only one patch when
> >> klp_target_state == KLP_UNPATCHED and
> >> klp_transition_patch->forced == true.
> > 
> > I probably misunderstand, but the above is not generally true, is it? I 
> > mean, if the transition patch is forced during its disablement, it does 
> > not say anything about the amount of enabled patches.
> > 
> >> But it is a bit tricky. I would do it the other way:
> >>
> >> 	if (klp_transition_patch->replace) {
> >> 		klp_for_each_patch(patch)
> >> 			patch->forced = true;
> >> 	} else if (klp_target_state == KLP_UNPATCHED) {
> >> 		klp_transition_patch->forced = true;
> >> 	}
> >>
> >> It looks more sane. And it makes it more clear
> >> that the special handling of KLP_UNPATCHED transition
> >> is done only when the atomic replace is not used.
> > 
> > But it is not the same. ->replace being true only comes into play when a 
> > patch is enabled. If it is disabled, then it behaves like any other patch.
> > 
> > So, if there is ->replace patch enabled (and it is the only patch present) 
> > and then more !->replace patches are loaded and then if ->replace patch is 
> > disabled and forced, your proposal would give a different result than what 
> > Chengming submitted, because in your case all the other patches will get 
> > ->forced set to true, while it is not the case in the original. It would 
> > be an unnecessary restriction if I am not missing something.
> 
> At first glance, I thought both way is right. But after looking at the case
> you mentioned above, they are not the same indeed. The original patch
> treat ->replace and not ->replace patches the same in KLP_UNPATCHED transition,
> and only set all patches to forced in the atomic replace transition.

I see. OK, Chengming's code makes sense. But we should make the commit
message more clear. Something like:

<draft>
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 "force" 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.
</draft>

It means that we should could actually do:

	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;
		}
	}

Huh, that is tricky ;-)

Best Regards,
Petr

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

[Chengming Zhou]

On 2022/3/11 12:30 上午, Petr Mladek wrote:
> On Thu 2022-03-10 20:57:54, Chengming Zhou wrote:
>> Hi,
>>
>> On 2022/3/9 1:49 上午, Miroslav Benes wrote:
>>> On Tue, 8 Mar 2022, Petr Mladek wrote:
>>>
>>>> On Thu 2022-03-03 18:54:46, Chengming Zhou wrote:
>>>>> module_put() is currently never called for a patch with forced flag, to block
>>>>> the removal of that patch module that might still be in use after a forced
>>>>> transition.
>>>>>
>>>>> But klp_force_transition() will set all patches on the list to be forced, since
>>>>> commit d67a53720966 ("livepatch: Remove ordering (stacking) of the livepatches")
>>>>> has removed stack ordering of the livepatches, it will cause all other patches can't
>>>>> be unloaded after disabled even if they have completed the KLP_UNPATCHED transition.
>>>>>
>>>>> In fact, we don't need to set a patch to forced if it's a KLP_PATCHED forced
>>>>> transition. It can still be unloaded safely as long as it has passed through
>>>>> the consistency model in KLP_UNPATCHED transition.
>>>>
>>>> It really looks safe. klp_check_stack_func() makes sure that @new_func
>>>> is not on the stack when klp_target_state == KLP_UNPATCHED. As a
>>>> result, the system should not be using code from the livepatch module
>>>> when KLP_UNPATCHED transition cleanly finished.
>>>>
>>>>
>>>>> But the exception is when force transition of an atomic replace patch, we
>>>>> have to set all previous patches to forced, or they will be removed at
>>>>> the end of klp_try_complete_transition().
>>>>>
>>>>> This patch only set the klp_transition_patch to be forced in KLP_UNPATCHED
>>>>> case, and keep the old behavior when in atomic replace case.
>>>>>
>>>>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>>>>> ---
>>>>> v2: interact nicely with the atomic replace feature noted by Miroslav.
>>>>> ---
>>>>>  kernel/livepatch/transition.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
>>>>> index 5683ac0d2566..34ffb8c014ed 100644
>>>>> --- a/kernel/livepatch/transition.c
>>>>> +++ b/kernel/livepatch/transition.c
>>>>> @@ -641,6 +641,10 @@ 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;
>>>>> +	if (klp_target_state == KLP_UNPATCHED)
>>>>> +		klp_transition_patch->forced = true;
>>>>> +	else if (klp_transition_patch->replace) {
>>>>> +		klp_for_each_patch(patch)
>>>>> +			patch->forced = true;
>>>>
>>>> This works only because there is should be only one patch when
>>>> klp_target_state == KLP_UNPATCHED and
>>>> klp_transition_patch->forced == true.
>>>
>>> I probably misunderstand, but the above is not generally true, is it? I 
>>> mean, if the transition patch is forced during its disablement, it does 
>>> not say anything about the amount of enabled patches.
>>>
>>>> But it is a bit tricky. I would do it the other way:
>>>>
>>>> 	if (klp_transition_patch->replace) {
>>>> 		klp_for_each_patch(patch)
>>>> 			patch->forced = true;
>>>> 	} else if (klp_target_state == KLP_UNPATCHED) {
>>>> 		klp_transition_patch->forced = true;
>>>> 	}
>>>>
>>>> It looks more sane. And it makes it more clear
>>>> that the special handling of KLP_UNPATCHED transition
>>>> is done only when the atomic replace is not used.
>>>
>>> But it is not the same. ->replace being true only comes into play when a 
>>> patch is enabled. If it is disabled, then it behaves like any other patch.
>>>
>>> So, if there is ->replace patch enabled (and it is the only patch present) 
>>> and then more !->replace patches are loaded and then if ->replace patch is 
>>> disabled and forced, your proposal would give a different result than what 
>>> Chengming submitted, because in your case all the other patches will get 
>>> ->forced set to true, while it is not the case in the original. It would 
>>> be an unnecessary restriction if I am not missing something.
>>
>> At first glance, I thought both way is right. But after looking at the case
>> you mentioned above, they are not the same indeed. The original patch
>> treat ->replace and not ->replace patches the same in KLP_UNPATCHED transition,
>> and only set all patches to forced in the atomic replace transition.
> 
> I see. OK, Chengming's code makes sense. But we should make the commit
> message more clear. Something like:
> 
> <draft>
> 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 "force" 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.
> </draft>

Ok, I will update the commit message, this draft is more clear.

> 
> It means that we should could actually do:
> 
> 	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;
> 		}
> 	}
> 
> Huh, that is tricky ;-)

Yes, and I found similar tricky code at the end of
klp_try_complete_transition():

	if (!patch->enabled)
		klp_free_patch_async(patch);
	else if (patch->replace)
		klp_free_replaced_patches_async(patch);

Thanks.

> 
> Best Regards,
> Petr