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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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
