Message-ID: <3e93e7b5-3077-9f03-ae89-55394e70da39@virtuozzo.com>
Date:   Fri, 26 Jan 2018 13:33:04 +0300
From:   Evgenii Shatokhin <eshatokhin@...tuozzo.com>
To:     Petr Mladek <pmladek@...e.com>, jpoimboe@...hat.com,
        jikos@...nel.org, mbenes@...e.cz
Cc:     Jason Baron <jbaron@...mai.com>, jeyu@...nel.org,
        linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On 25.01.2018 19:02, Petr Mladek wrote:
> From: Jason Baron <jbaron@...mai.com>
> 
> Sometimes we would like to revert a particular fix. Currently, this
> is not easy because we want to keep all other fixes active and we
> could revert only the last applied patch.
> 
> One solution would be to apply new patch that implemented all
> the reverted functions like in the original code. It would work
> as expected but there will be unnecessary redirections. In addition,
> it would also require knowing which functions need to be reverted at
> build time.
> 
> Another problem is when there are many patches that touch the same
> functions. There might be dependencies between patches that are
> not enforced on the kernel side. Also it might be pretty hard to
> actually prepare the patch and ensure compatibility with
> the other patches.
> 
> A better solution would be to create cumulative patch and say that
> it replaces all older ones.
> 
> This patch adds a new "replace" flag to struct klp_patch. When it is
> enabled, a set of 'nop' klp_func will be dynamically created for all
> functions that are already being patched but that will not longer be
> modified by the new patch. They are temporarily used as a new target
> during the patch transition.
> 
> There are used several simplifications:
> 
>    + nops' structures are generated already when the patch is registered.
>      All registered patches are taken into account, even the disabled ones.
>      As a result, there might be more nops than are really needed when
>      the patch is enabled and some disabled patches were removed before.
>      But we are on the safe side and it simplifies the implementation.
>      Especially we could reuse the existing init() functions. Also freeing
>      is easier because the same nops are created and removed only once.
> 
>      Alternative solution would be to create nops when the patch is enabled.
>      But then we would need to complicated to reuse the init() functions
>      and error paths. It would increase the risk of errors because of
>      late kobject initialization. It would need tricky waiting for
>      freed kobjects when finalizing a reverted enable transaction.
> 
>    + The replaced patches are removed from the stack and cannot longer
>      be enabled directly. Otherwise, we would need to implement a more
>      complex logic of handling the stack of patches. It might be hard
>      to come with a reasonable semantic.
> 
>      A fallback is to remove (rmmod) the replaced patches and register
>      (insmod) them again.
> 
>    + Nops are handled like normal function patches. It reduces changes
>      in the existing code.
> 
>      It would be possible to copy internal values when they are allocated
>      and make short cuts in init() functions. It would be possible to use
>      the fact that old_func and new_func point to the same function and
>      do not init new_func and new_size at all. It would be possible to
>      detect nop func in ftrace handler and just leave. But all these would
>      just complicate the code and maintenance.
> 
>    + The callbacks from the replaced patches are not called. It would be
>      pretty hard to define a reasonable semantic and implement it.

At least, it surely simplifies error handling, if these callbacks are 
not called.

Anyway, I guess, this restriction should be mentioned explicitly in the 
docs. I think this is not obvious for the patch developers (esp. those 
familiar with RPM spec files and such ;-) ).

What concerns me is that downgrading of the cumulative patches with 
callbacks becomes much more difficult this way.

I mean, suppose a user has v1 of a cumulative patch installed. Then a 
newer version, v2, is released. They install it and find that it is 
buggy (very unfortunate but might still happen). Now they cannot 
atomically replace v2 back with v1, because the callbacks from v1 cannot 
clean up after v2.

It will be needed to unload v2 explicitly and then load v1 back, which 
is more fragile. The loading failures are much more unlikely with 
livepatch than with the old kpatch, but they are still possible.

I have no good solution to this though.

> 
>      It might even be counter-productive. The new patch is cumulative.
>      It is supposed to include most of the changes from older patches.
>      In most cases, it will not want to call pre_unpatch() post_unpatch()
>      callbacks from the replaced patches. It would disable/break things
>      for no good reasons. Also it should be easier to handle various
>      scenarios in a single script in the new patch than think about
>      interactions caused by running many scripts from older patches.
>      No to say that the old scripts even would not expect to be called
>      in this situation.
> 
> Signed-off-by: Jason Baron <jbaron@...mai.com>
> [pmladek@...e.com: Split, reuse existing code, simplified]
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> Cc: Jessica Yu <jeyu@...nel.org>
> Cc: Jiri Kosina <jikos@...nel.org>
> Cc: Miroslav Benes <mbenes@...e.cz>
> Cc: Petr Mladek <pmladek@...e.com>
> ---
>   include/linux/livepatch.h     |   3 +
>   kernel/livepatch/core.c       | 162 +++++++++++++++++++++++++++++++++++++++++-
>   kernel/livepatch/core.h       |   4 ++
>   kernel/livepatch/transition.c |  36 ++++++++++
>   4 files changed, 203 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 21cad200f949..9d44d105b278 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -42,6 +42,7 @@
>   enum klp_func_type {
>   	KLP_FUNC_ANY = -1,	/* Substitute any type */
>   	KLP_FUNC_ORIGINAL = 0,  /* Original statically defined structure */
> +	KLP_FUNC_NOP,		/* Dynamically allocated NOP function patch */
>   };
>   
>   /**
> @@ -153,6 +154,7 @@ struct klp_object {
>    * struct klp_patch - patch structure for live patching
>    * @mod:	reference to the live patch module
>    * @objs:	object entries for kernel objects to be patched
> + * @replace:	replace all already registered patches
>    * @list:	list node for global list of registered patches
>    * @kobj:	kobject for sysfs resources
>    * @obj_list:	head of list for struct klp_object
> @@ -163,6 +165,7 @@ struct klp_patch {
>   	/* external */
>   	struct module *mod;
>   	struct klp_object *objs;
> +	bool replace;
>   
>   	/* internal */
>   	struct list_head list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6ad3195d995a..c606b291dfcd 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,21 @@ static bool klp_initialized(void)
>   	return !!klp_root_kobj;
>   }
>   
> +static struct klp_func *klp_find_func(struct klp_object *obj,
> +				      struct klp_func *old_func)
> +{
> +	struct klp_func *func;
> +
> +	klp_for_each_func(obj, func) {
> +		if ((strcmp(old_func->old_name, func->old_name) == 0) &&
> +		    (old_func->old_sympos == func->old_sympos)) {
> +			return func;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>   static struct klp_object *klp_find_object(struct klp_patch *patch,
>   					  struct klp_object *old_obj)
>   {
> @@ -342,6 +357,39 @@ static int klp_write_object_relocations(struct module *pmod,
>   	return ret;
>   }
>   
> +/*
> + * This function removes replaced patches from both func_stack
> + * and klp_patches stack.
> + *
> + * We could be pretty aggressive here. It is called in situation
> + * when these structures are not longer accessible. All functions
> + * are redirected using the klp_transition_patch. They use either
> + * a new code or they in the original code because of the special
> + * nop function patches.
> + */
> +void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
> +				     bool keep_module)
> +{
> +	struct klp_patch *old_patch, *tmp_patch;
> +
> +	list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) {
> +		if (old_patch == new_patch)
> +			return;
> +
> +		klp_unpatch_objects(old_patch, KLP_FUNC_ANY);
> +		old_patch->enabled = false;
> +
> +		/*
> +		 * Replaced patches could not get re-enabled to keep
> +		 * the code sane.
> +		 */
> +		list_del_init(&old_patch->list);
> +
> +		if (!keep_module)
> +			module_put(old_patch->mod);
> +	}
> +}
> +
>   static int __klp_disable_patch(struct klp_patch *patch)
>   {
>   	struct klp_object *obj;
> @@ -537,7 +585,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	if (!klp_is_patch_usable(patch)) {
>   		/*
>   		 * Module with the patch could either disappear meanwhile or is
> -		 * not properly initialized yet.
> +		 * not properly initialized yet or the patch was just replaced.
>   		 */
>   		ret = -EINVAL;
>   		goto err;
> @@ -662,8 +710,16 @@ static struct attribute *klp_patch_attrs[] = {
>   /*
>    * Dynamically allocated objects and functions.
>    */
> +static void klp_free_func_nop(struct klp_func *func)
> +{
> +	kfree(func->old_name);
> +	kfree(func);
> +}
> +
>   static void klp_free_func_dynamic(struct klp_func *func)
>   {
> +	if (func->ftype == KLP_FUNC_NOP)
> +		klp_free_func_nop(func);
>   }
>   
>   static void klp_free_object_dynamic(struct klp_object *obj)
> @@ -691,7 +747,7 @@ static struct klp_object *klp_alloc_object_dynamic(const char *name)
>   	return obj;
>   }
>   
> -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)
>   {
>   	struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
>   	return obj;
>   }
>   
> +static struct klp_func *klp_alloc_func_nop(struct klp_func *old_func,
> +					   struct klp_object *obj)
> +{
> +	struct klp_func *func;
> +
> +	func = kzalloc(sizeof(*func), GFP_KERNEL);
> +	if (!func)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (old_func->old_name) {
> +		func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
> +		if (!func->old_name) {
> +			kfree(func);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +	func->old_sympos = old_func->old_sympos;
> +	/* NOP func is the same as using the original implementation. */
> +	func->new_func = (void *)old_func->old_addr;
> +	func->ftype = KLP_FUNC_NOP;
> +
> +	return func;
> +}
> +
> +static int klp_add_func_nop(struct klp_object *obj,
> +			    struct klp_func *old_func)
> +{
> +	struct klp_func *func;
> +
> +	func = klp_find_func(obj, old_func);
> +
> +	if (func)
> +		return 0;
> +
> +	func = klp_alloc_func_nop(old_func, obj);
> +	if (IS_ERR(func))
> +		return PTR_ERR(func);
> +
> +	klp_init_func_list(obj, func);
> +
> +	return 0;
> +}
> +
> +static int klp_add_object_nops(struct klp_patch *patch,
> +			       struct klp_object *old_obj)
> +{
> +	struct klp_object *obj;
> +	struct klp_func *old_func;
> +	int err = 0;
> +
> +	obj = klp_get_or_add_object(patch, old_obj);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +
> +	klp_for_each_func(old_obj, old_func) {
> +		err = klp_add_func_nop(obj, old_func);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Add 'nop' functions which simply return to the caller to run
> + * the original function. The 'nop' functions are added to a
> + * patch to facilitate a 'replace' mode
> + *
> + * The nops are generated for all patches on the stack when
> + * the new patch is initialized. It is safe even though some
> + * older patches might get disabled and removed before the
> + * new one is enabled. In the worst case, there might be nops
> + * there will not be really needed. But it does not harm and
> + * simplifies the implementation a lot. Especially we could
> + * use the init functions as is.
> + */
> +static int klp_add_nops(struct klp_patch *patch)
> +{
> +	struct klp_patch *old_patch;
> +	struct klp_object *old_obj;
> +	int err = 0;
> +
> +	if (WARN_ON(!patch->replace))
> +		return -EINVAL;
> +
> +	list_for_each_entry(old_patch, &klp_patches, list) {
> +		klp_for_each_object(old_patch, old_obj) {
> +			err = klp_add_object_nops(patch, old_obj);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * Patch release framework must support the following scenarios:
>    *
> @@ -952,6 +1104,12 @@ static int klp_init_patch(struct klp_patch *patch)
>   		return ret;
>   	}
>   
> +	if (patch->replace) {
> +		ret = klp_add_nops(patch);
> +		if (ret)
> +			goto free;
> +	}
> +
>   	klp_for_each_object(patch, obj) {
>   		ret = klp_init_object(patch, obj);
>   		if (ret)
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 48a83d4364cf..43184a5318d8 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -6,6 +6,10 @@
>   
>   extern struct mutex klp_mutex;
>   
> +void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
> +				     bool keep_module);
> +void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype);
> +
>   static inline bool klp_is_object_loaded(struct klp_object *obj)
>   {
>   	return !obj->name || obj->mod;
> 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();
> +
> +		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
> @@ -143,6 +170,15 @@ static void klp_complete_transition(void)
>   	if (!klp_forced && klp_target_state == KLP_UNPATCHED)
>   		module_put(klp_transition_patch->mod);
>   
> +	/*
> +	 * We do not need to wait until the objects are really freed.
> +	 * The patch must be on the bottom of the stack. Therefore it
> +	 * will never replace anything else. The only important thing
> +	 * is that we wait when the patch is being unregistered.
> +	 */
> +	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
> +		klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
> +
>   	klp_target_state = KLP_UNDEFINED;
>   	klp_transition_patch = NULL;
>   }
> 

Powered by blists - more mailing lists