Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace
To:     Petr Mladek <pmladek@suse.com>, jpoimboe@redhat.com,
        jikos@kernel.org, mbenes@suse.cz
Cc:     jeyu@kernel.org, Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
        linux-kernel@vger.kernel.org, live-patching@vger.kernel.org
References: <20180125160203.28959-1-pmladek@suse.com>
 <20180125160203.28959-7-pmladek@suse.com>
From:   Jason Baron <jbaron@akamai.com>
Message-ID: <693db35b-3bc8-d5de-207e-6321226d7f58@akamai.com>
Date:   Thu, 25 Jan 2018 23:27:57 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20180125160203.28959-7-pmladek@suse.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 01/25/2018 11:02 AM, Petr Mladek wrote:
> From: Jason Baron <jbaron@akamai.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.
> 
>     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@akamai.com>
> [pmladek@suse.com: Split, reuse existing code, simplified]

Hi Petr,

Thanks for cleaning this up - it looks good.
I just had one comment/issue below thus far.


> Signed-off-by: Petr Mladek <pmladek@suse.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Miroslav Benes <mbenes@suse.cz>
> Cc: Petr Mladek <pmladek@suse.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);

I'm wondering if this should be:

list_move(&old_patch->list, &klp_replaced_patches);

Which ensures that the only valid transition after a patch has been
'replaced' is an 'unregister'.

Otherwise, klp_replaced_patches is not used anywhere.

Thanks,

-Jason


> +
> +		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;
>  }
>