[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LSU.2.21.1802011423300.26656@pobox.suse.cz>
Date: Thu, 1 Feb 2018 14:49:24 +0100 (CET)
From: Miroslav Benes <mbenes@...e.cz>
To: Petr Mladek <pmladek@...e.com>
cc: jpoimboe@...hat.com, jikos@...nel.org,
Jason Baron <jbaron@...mai.com>, jeyu@...nel.org,
Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace
> -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> struct klp_object *old_obj)
A nit, but this change belongs to 3/6, doesn't it?
> {
> struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> return obj;
> }
[...]
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 6917100fbe79..3f6cf5b9e048 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
> klp_transition_patch->mod->name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> + /*
> + * For replace patches, we disable all previous patches, and replace
> + * the dynamic no-op functions by removing the ftrace hook.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> + /*
> + * Make sure klp_ftrace_handler() can no longer see functions
> + * from the replaced patches. Then all functions will be
> + * redirected using klp_transition_patch. They will use
> + * either a new code or they will stay in the original code
> + * because of the nop funcs' structures.
> + */
> + if (klp_forced)
> + klp_synchronize_transition();
I'm not sure why this is here. klp_forced should not be so special that it
would warrant a synchronization. However, I'm also not sure that it would
be safe to remove it from the patch.
Is this because "force" is the only one who can clear TIF_PATCH_PENDING of
other tasks? So, there could be a task running in klp_ftrace_handler(),
its TIF_PATCH_PENDING is cleared by force and we could have a problem
because of that since we're throwing away replaced patches.
Ok, that sounds viable. I'd welcome a comment on that in the code.
Thanks,
Miroslav
> +
> + klp_throw_away_replaced_patches(klp_transition_patch,
> + klp_forced);
> +
> + /*
> + * There is no need to synchronize the transition after removing
> + * nops. They must be the last on the func_stack. Ftrace
> + * gurantees that nobody will stay in the trampoline after
> + * the ftrace handler is unregistered.
> + */
> + klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> + }
> +
> if (klp_target_state == KLP_UNPATCHED) {
> /*
> * All tasks have transitioned to KLP_UNPATCHED so we can now
Powered by blists - more mailing lists