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 <pmla...@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
