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

[Chengming Zhou <zhouchengming@bytedance.com>]

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