Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

[Petr Mladek <pmladek@suse.com>]

On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..aca62c4b8616 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
>  	return obj->name;
>  }
>  
> -static bool klp_is_object_loaded(struct klp_object *obj)
> -{
> -	return !obj->name || obj->mod;
> -}
> -
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {
> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
>  
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
> +	struct klp_object *obj;
> +
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  
>  	klp_init_transition(patch, KLP_UNPATCHED);
>  
> +	klp_for_each_object(patch, obj)
> +		if (patch->enabled && obj->patched)
> +			klp_pre_unpatch_callback(obj);
> +
>  	/*
>  	 * Enforce the order of the func->transition writes in
>  	 * klp_init_transition() and the TIF_PATCH_PENDING writes in
> @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  		if (!klp_is_object_loaded(obj))
>  			continue;
>  
> -		ret = klp_patch_object(obj);
> +		ret = klp_pre_patch_callback(obj);
>  		if (ret) {
> -			pr_warn("failed to enable patch '%s'\n",
> -				patch->mod->name);
> +			pr_warn("pre-patch callback failed for object '%s'\n",
> +				klp_is_module(obj) ? obj->name : "vmlinux");
> +			goto err;
> +		}
>  
> -			klp_cancel_transition();
> -			return ret;
> +		ret = klp_patch_object(obj);
> +		if (ret) {
> +			pr_warn("failed to patch object '%s'\n",
> +				klp_is_module(obj) ? obj->name : "vmlinux");

We should call klp_post_unpatch_callback(obj) here to make it
synchronous.

Well, what about calling:

      klp_pre_patch_callback() inside klp_patch_object() and
      klp_post_unpatch_callback() inside klp_unpatch_object()

By other words, we would do the two operations. It would have
two advantages:

   + error handling for free
   + no need for the strange callbacks_enabled flag

It would require the more strict consistency model if there
is a dependency between the callbacks and patches from various
modules. But we would probably need the consistency model
in this case anyway.

> +			goto err;

>  		}
>  	}
>  

Otherwise I think that we are getting close.

Best Regards,
Petr

PS: I hope that the above problem and solution has not been mentioned
yet. I am sorry if it was. I am a bit lost in many mails after
vacation, sickness, and conference.