Re: [PATCH v3 2/2] livepatch: add atomic replace

[Jason Baron]

On 10/18/2017 07:25 AM, Petr Mladek wrote:
> On Wed 2017-10-18 11:10:09, Miroslav Benes wrote:
>> On Tue, 17 Oct 2017, Jason Baron wrote:
>>> If the atomic replace patch does
>>> not contain any immediates, then we can drop the reference on the
>>> immediately preceding patch only. That is because there may have been
>>> previous transitions to immediate functions in the func stack, and the
>>> transition to the atomic replace patch only checks immediately preceding
>>> transition. It would be possible to check all of the previous immediate
>>> function transitions, but this adds complexity and seems like not a
>>> common pattern. So I would suggest that we just drop the reference on
>>> the previous patch if the atomic replace patch does not contain any
>>> immediate functions.
>>
>> It is even more complicated and it is not connected only to atomic replace 
>> patch (I realized this while reading the first part of your email and 
>> then you confirmed it with this paragraph). The consistency model is 
>> broken with respect to immediate patches.
>>
>> func		a
>> patches		1i
>> 		2i
>> 		3
>>
>> Now, when you're applying 3, only 2i function is checked. But there might 
>> be a task sleeping in 1i. Such task would be migrated to 3, because we do 
>> not check 1 in klp_check_stack_func() at all.
>>
>> I see three solutions.
>>
>> 1. Say it is an user's fault. Since it is not obvious and it is 
>> easy-to-make mistake, I would not go this way.
>>
>> 2. We can fix klp_check_stack_func() in an exact way you're proposing. 
>> We'd go back in func stack as long as there are immediate patches there. 
>> This adds complexity and I'm not sure if all the problems would be solved 
>> because scenarios how patches are stacked and applied to different 
>> functions may be quite complex.
>>
>> 3. Drop immediate. It causes problems only and its advantages on x86_64 
>> are theoretical. You would still need to solve the interaction with atomic 
>> replace on other architecture with immediate preserved, but that may be 
>> easier. Or we can be aggressive and drop immediate completely. The force 
>> transition I proposed earlier could achieve the same.
> 
> To make it clear. We currently rely on the immediate handling on
> architectures without a reliable stack checking. The question
> is if anyone uses it for another purpose in practice.
> 
> A solution would be to remove the per-func immediate flag
> and invert the logic of the per-patch one. We could rename
> it to something like "consistency_required" or "semantic_changes".
> A patch with this flag set then might be refused on systems
> without reliable stacks. Otherwise, the consistency model
> would be used for all patches.
> 
> As a result, all patches would be handled either using
> the consistency model or immediately. We would need to
> care about any mix of these.
> 
> Best Regards,
> Petr
> 

Hi,

So for atomic replace, it seems as if we don't want to allow replaced
patches to be re-enabled but instead, allow them to rmmod/insmod'ed to
allow revert.

In your above proposed model around immediate, if all patches are
immediate then the atomic replace doesn't allow any of the previous
patches to be removed from the kernel via rmmod, while if all patches
are handled using the consistency model then all patches previous to the
atomic replace patch could be removed via rmmod. So this would simplify
the logic around when replaced patches could removed.

I was thinking in the current model to only allow the one preceding
patch to the atomic replace patch to be rmmod'able, if the atomic
replace patch was using the consistency model (to avoid complications
with immediate functions/patches). And this could be extended to all
*all* patches previous to the atomic replace patch to be rmmod'able when
we move to the proposed model around immediates. So I don't think this
should hold up the atomic replace patches?

Thanks,

-Jason