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

[Jason Baron]

On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
>> Since 'atomic replace' has completely replaced all previous livepatch
>> modules, it explicitly disables all previous livepatch modules. However,
>> previous livepatch modules that have been replaced, can be re-enabled
>> if they have the 'replace' flag set. In this case the set of 'nop'
>> functions is re-calculated, such that it replaces all others.
>>
>> For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set
>> then:
>>
>> # insmod kpatch-a.ko
>> # insmod kpatch-b.ko
>>
>> At this point we have:
>>
>> # cat /sys/kernel/livepatch/kpatch-a/enabled
>> 0
>>
>> # cat /sys/kernel/livepatch/kpatch-b/enabled
>> 1
>>
>> To revert to the kpatch-a state we can do:
>>
>> echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
>>
>> And now we have:
>>
>> # cat /sys/kernel/livepatch/kpatch-a/enabled
>> 1
>>
>> # cat /sys/kernel/livepatch/kpatch-b/enabled
>> 0
> 
> I don't really like allowing a previously replaced patch to replace the
> current patch.  It's just more unnecessary complexity.  If the user
> wants to atomically revert back to kpatch-a, they should be able to:
> 
>   rmmod kpatch-a
>   insmod kpatch-a.ko
>

Right - that's how I sent v1 (using rmmod/insmod to revert), but it
didn't account for the fact the patch or some functions may be marked
'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
some cases 'rmmod' was not feasible, I thought it would be simpler from
an operational pov to just say we always revert by re-enabling a
previously replaced patch as opposed to rmmod/insmod.


>> Note that it may be possible to unload (rmmod) replaced patches in some
>> cases based on the consistency model, when we know that all the functions
>> that are contained in the patch may no longer be in used, however its
>> left as future work, if this functionality is desired.
> 
> If you don't allow a previously replaced patch to be enabled again, I
> think it would be trivial to let it be unloaded.
>

The concern is around being replaced by 'immediate' functions and thus
not knowing if the code is still in use.

>> Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving
>> a bunch of existing functions before __klp_enable_patch(). So there is a
>> bit of churn in moving functions that are not modified.
> 
> To make review easier, can you put the moving of functions into a
> separate patch?
> 

Agreed - I've re-posted to this thread as v3.1.

Thanks,

-Jason