Date: Tue, 26 Sep 2017 16:49:12 +0200
From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@kernel.org>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks
Message-ID: <20170926144912.GH21048@pathway.suse.cz>
References: <1504191233-2642-1-git-send-email-joe.lawrence@redhat.com>
 <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2684
Lines: 91

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.