Re: [PATCH v8 0/8] livepatch: Atomic replace feature

[Evgenii Shatokhin]

Hi,

Petr, Jason - thanks a lot for working on this series, first of all! And 
especially for your patience.

On 21.02.2018 16:29, Petr Mladek wrote:
> The atomic replace allows to create cumulative patches. They
> are useful when you maintain many livepatches and want to remove
> one that is lower on the stack. In addition it is very useful when
> more patches touch the same function and there are dependencies
> between them.

I have experimented with this updated patchset a bit.
It looks like replace operation fails if there is a loaded but disabled 
patch.

Suppose there are 2 binary patches changing the same kernel functions. 
Load patch1, then disable it (echo 0 > 
/sys/kernel/livepatch/patch1/enabled), then try load patch2 with atomic 
replace. I get "Device or resource busy" error at this point.

It seems, __klp_enable_patch() returns -EBUSY for some reason. I haven't 
dug deeper into that code yet.

A workaround is simple: just unload all disabled patches before trying 
to load patch2 with replace. Still, the behavior is quite strange.

> I have found one bug in v7. We were not able to initialize NOP
> struct klp_func when the patches module is not loaded. It was
> because func->new_func was NULL. I have fixed it in separate patch
> for an easier review.
>
> Note that the original Jason's patch did not have this problem
> because func->new_func was always NULL there. But this required
> NOP-specific handling also on other locations, namely
> klp_ftrace_handler() and klp_check_stack_func().
>
> Changes against v7:
>
>    + Fixed handling of NOPs for not-yet-loaded modules
>    + Made klp_replaced_patches list static [Mirek]
>    + Made klp_free_object() public later [Mirek]
>    + Fixed several reported typos [Mirek, Joe]
>    + Updated documentation according to the feedback [Joe]
>    + Added some Acks [Mirek]
>
> Changes against v6:
>
>    + used list_move when disabling replaced patches [Jason]
>    + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek]
>    + used klp_is_func_type() in klp_unpatch_object() [Mirek]
>    + moved static definition of klp_get_or_add_object() [Mirek]
>    + updated comment about synchronization in forced mode [Mirek]
>    + added user documentation
>    + fixed several typos
>
> Jason Baron (5):
>    livepatch: Use lists to manage patches, objects and functions
>    livepatch: Initial support for dynamic structures
>    livepatch: Allow to unpatch only functions of the given type
>    livepatch: Support separate list for replaced patches.
>    livepatch: Add atomic replace
>
> Petr Mladek (3):
>    livepatch: Free only structures with initialized kobject
>    livepatch: Correctly handle atomic replace for not yet loaded modules
>    livepatch: Atomic replace and cumulative patches documentation
>
>   Documentation/livepatch/cumulative-patches.txt |  83 ++++++
>   include/linux/livepatch.h                      |  59 +++-
>   kernel/livepatch/core.c                        | 394 ++++++++++++++++++++++---
>   kernel/livepatch/core.h                        |   4 +
>   kernel/livepatch/patch.c                       |  31 +-
>   kernel/livepatch/patch.h                       |   4 +-
>   kernel/livepatch/transition.c                  |  41 ++-
>   7 files changed, 566 insertions(+), 50 deletions(-)
>   create mode 100644 Documentation/livepatch/cumulative-patches.txt

Regards,
Evgenii