Re: [PATCH] livepatch: Fix the issue to make livepatch enable/disable patch correctly

[Minfei Huang <mnfhuang@gmail.com>]

Could someone help to review this patch?

Thanks
Minfei

On 07/15/15 at 04:55pm, Minfei Huang wrote:
> From: Minfei Huang <mnfhuang@gmail.com>
> 
> Livepatch will obey the stacking rule to enable/disable the patch. It
> only allows to enable the patch, when it is the fist disabled patch,
> disable the patch, when it is the last enabled patch.
> 
> In the livepatch code, it uses list to gather the all of the patches.
> And we do not know whether the previous/next patch is patched to the
> same modules or vmlinux in that way.
> 
> According to above rule, livepatch will make incorrect decision to
> enable/disable the patch. Following is an example to show how livepatch
> does.
> 
> - install the livepatch example module which is in samples/livepatch.
> - install the third part kernel module
> - install the livepatch module which is patched to the third part module
> - disable the livepatch example module
> 
> We can find that we can not disable livepatch example module, although
> it is the last enabled patch.
> 
> To fix this issue, we will find the corresponding patch which is patched
> to the same modules or vmlinux, when we enable/disable the patch.
> 
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
>  kernel/livepatch/core.c | 55 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..d59aec7 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -429,6 +429,27 @@ err:
>  	return ret;
>  }
>  
> +static bool is_patched_to_same(struct klp_patch *p1, struct klp_patch *p2)
> +{
> +	struct klp_object *obj1, *obj2;
> +	bool is_mod1, is_mod2;
> +
> +	klp_for_each_object(p1, obj1) {
> +		klp_for_each_object(p2, obj2) {
> +			is_mod1 = !!klp_is_module(obj1);
> +			is_mod2 = !!klp_is_module(obj2);
> +
> +			if (is_mod1 && is_mod2) {
> +				if (!strcmp(obj1->name, obj2->name))
> +					return true;
> +			} else if (!is_mod1 && !is_mod2)
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static void klp_disable_object(struct klp_object *obj)
>  {
>  	struct klp_func *func;
> @@ -463,13 +484,26 @@ static int klp_enable_object(struct klp_object *obj)
>  	return 0;
>  }
>  
> +struct klp_patch *get_next_patch(struct klp_patch *patch)
> +{
> +	struct klp_patch *p = patch;
> +
> +	list_for_each_entry_continue(p, &klp_patches, list) {
> +		if (is_patched_to_same(p, patch))
> +			return p;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
> +	struct klp_patch *p;
>  	struct klp_object *obj;
>  
>  	/* enforce stacking: only the last enabled patch can be disabled */
> -	if (!list_is_last(&patch->list, &klp_patches) &&
> -	    list_next_entry(patch, list)->state == KLP_ENABLED)
> +	p = get_next_patch(patch);
> +	if (p && (p->state == KLP_ENABLED))
>  		return -EBUSY;
>  
>  	pr_notice("disabling patch '%s'\n", patch->mod->name);
> @@ -516,8 +550,21 @@ err:
>  }
>  EXPORT_SYMBOL_GPL(klp_disable_patch);
>  
> +struct klp_patch *get_prev_patch(struct klp_patch *patch)
> +{
> +	struct klp_patch *p = patch;
> +
> +	list_for_each_entry_continue_reverse(p, &klp_patches, list) {
> +		if (is_patched_to_same(p, patch))
> +			return p;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int __klp_enable_patch(struct klp_patch *patch)
>  {
> +	struct klp_patch *p;
>  	struct klp_object *obj;
>  	int ret;
>  
> @@ -525,8 +572,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  		return -EINVAL;
>  
>  	/* enforce stacking: only the first disabled patch can be enabled */
> -	if (patch->list.prev != &klp_patches &&
> -	    list_prev_entry(patch, list)->state == KLP_DISABLED)
> +	p = get_prev_patch(patch);
> +	if (p && (p->state == KLP_DISABLED))
>  		return -EBUSY;
>  
>  	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> -- 
> 2.2.2
>