lkml.org 
[lkml]   [2018]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules
    On Wed, 21 Feb 2018, Petr Mladek wrote:

    > The atomic replace feature uses dynamically allocated struct klp_func to
    > handle functions that will not longer be patched. These structures are

    s/not longer/no longer/, but "handle functions that will not be patched
    any longer" may be even better.

    > of the type KLP_FUNC_NOP. They cause the ftrace handler to jump to
    > the original code. But the address of the original code is not known
    > until the patched module is loaded.
    >
    > This patch allows the late initialization. Also it adds a sanity check
    > into the ftrace handler.
    >
    > Alternative solution would be to do not set the address at all. The ftrace

    "Alternative solution would be not to set the address at all." ?

    > handler could just return to the original code when NOP struct klp_func
    > is used. But this would require another changes. For example, in the stack
    > checking. Note that NOP structures might be available even when the patch
    > is being disabled. This would happen when the patch enable transition is
    > reverted.
    >
    > Signed-off-by: Petr Mladek <pmladek@suse.com>
    > ---
    > kernel/livepatch/core.c | 16 ++++++++++++++--
    > kernel/livepatch/patch.c | 5 +++++
    > 2 files changed, 19 insertions(+), 2 deletions(-)
    >
    > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
    > index ad508a86b2f9..da1438d47d83 100644
    > --- a/kernel/livepatch/core.c
    > +++ b/kernel/livepatch/core.c
    > @@ -945,8 +945,12 @@ static void klp_free_object_loaded(struct klp_object *obj)
    >
    > obj->mod = NULL;
    >
    > - klp_for_each_func(obj, func)
    > + klp_for_each_func(obj, func) {
    > func->old_addr = 0;
    > +
    > + if (klp_is_func_type(func, KLP_FUNC_NOP))
    > + func->new_func = NULL;
    > + }
    > }
    >
    > /*
    > @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
    >
    > static int klp_init_func(struct klp_object *obj, struct klp_func *func)
    > {
    > - if (!func->old_name || !func->new_func)
    > + if (!func->old_name)
    > + return -EINVAL;
    > +
    > + /* NOPs do not know the address until the patched module is loaded. */
    > + if (!func->new_func &&
    > + (!klp_is_func_type(func, KLP_FUNC_NOP) || klp_is_object_loaded(obj)))
    > return -EINVAL;

    If we changed the order of klp_init_func() and klp_init_object_loaded()
    calls in klp_init_object(), the hunk would not be needed. Is that correct?
    But that would require more changes elsewhere, so it's not worth it, I
    think.

    > INIT_LIST_HEAD(&func->stack_node);
    > @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
    > return -ENOENT;
    > }
    >
    > + if (klp_is_func_type(func, KLP_FUNC_NOP))
    > + func->new_func = (void *)func->old_addr;

    Is there a reason why you left the same assignment in
    klp_alloc_func_nop()? This one is enough, no?

    Miroslav

    \
     
     \ /
      Last update: 2018-02-22 22:01    [W:4.095 / U:0.364 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site