[PATCH 0/2] livepatch: Additional fixes for callbacks feature

[PATCH 0/2] livepatch: Additional fixes for callbacks feature

[Petr Mladek]

I have found few small problems when reviewing the patch adding
(un)patch callback, see
https://lkml.kernel.org/r/1507921723-1996-2-git-send-email-joe.lawrence@redhat.com
Feel free to merge them with the original patch if still possible.

Petr Mladek (2):
  livepatch: Correctly call klp_post_unpatch_callback() in error paths
  livepatch: __klp_disable_patch() should never be called for disabled
    patches

 kernel/livepatch/core.c | 9 +++++----
 kernel/livepatch/core.h | 8 +++++---
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
1.8.5.6

[PATCH 1/2] livepatch: Correctly call klp_post_unpatch_callback() in error paths

[Petr Mladek]

The flag post_unpatch_enabled in struct klp_callbacks is need in
the error paths. We need to call the post_unpatch callback
only when the pre_patch one was called.

We should clear the flag in klp_post_unpatch_callback() to make
sure that the callback is not called twice. It makes the API
more safe.

Note that we actually would call the callback twice in
klp_module_coming() when klp_patch_object() fails.
In this case, we explicitly call klp_post_unpatch_callback()
for the failed object. And we are going to call it once
again when reverting operations for all the patches by
reusing the klp_module_going() code. There is a patch
doing this in the queue.

There was another mistake in the error path in klp_comming_module().
It called klp_post_unpatch_callback() only when
patch != klp_transition_patch. But klp_pre_patch_callback()
was called even for this patch.

In fact, we could remove klp_post_unpatch_callback() from
the error path at all because it will be covered by
the reuse of the klp_module_going() code. But I think
that it is cleaner this way. For example, someone
might later decide to call the callback only when
obj->patched flag is set.

Finally, I used this opportunity to make klp_pre_patch_callback()
more readable.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 4 +---
 kernel/livepatch/core.h | 8 +++++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index cafb5a84417d..eb134479c394 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -894,9 +894,7 @@ int klp_module_coming(struct module *mod)
 				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
 					patch->mod->name, obj->mod->name, ret);
 
-				if (patch != klp_transition_patch)
-					klp_post_unpatch_callback(obj);
-
+				klp_post_unpatch_callback(obj);
 				goto err;
 			}
 
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 6fc907b54e71..cc3aa708e0b4 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -12,10 +12,10 @@ static inline bool klp_is_object_loaded(struct klp_object *obj)
 
 static inline int klp_pre_patch_callback(struct klp_object *obj)
 {
-	int ret;
+	int ret = 0;
 
-	ret = (obj->callbacks.pre_patch) ?
-		(*obj->callbacks.pre_patch)(obj) : 0;
+	if (obj->callbacks.pre_patch)
+		ret = (*obj->callbacks.pre_patch)(obj);
 
 	obj->callbacks.post_unpatch_enabled = !ret;
 
@@ -39,6 +39,8 @@ static inline void klp_post_unpatch_callback(struct klp_object *obj)
 	if (obj->callbacks.post_unpatch_enabled &&
 	    obj->callbacks.post_unpatch)
 		(*obj->callbacks.post_unpatch)(obj);
+
+	obj->callbacks.post_unpatch_enabled = false;
 }
 
 #endif /* _LIVEPATCH_CORE_H */
-- 
1.8.5.6

[PATCH 2/2] livepatch: __klp_disable_patch() should never be called for disabled patches

[Petr Mladek]

__klp_disable_patch() should never be called when the patch is not
enabled. Let's add the same warning that we have in __klp_enable_patch().

This allows to remove the check when calling klp_pre_unpatch_callback().
It was strange anyway because it repeatedly checked per-patch flag
for each patched object.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb134479c394..287f71e9dbfe 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -282,6 +282,9 @@ static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 
+	if (WARN_ON(!patch->enabled))
+		return -EINVAL;
+
 	if (klp_transition_patch)
 		return -EBUSY;
 
@@ -293,7 +296,7 @@ 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)
+		if (obj->patched)
 			klp_pre_unpatch_callback(obj);
 
 	/*
-- 
1.8.5.6

Re: [PATCH 2/2] livepatch: __klp_disable_patch() should never be called for disabled patches

[Joe Lawrence]

On Fri, Oct 20, 2017 at 04:56:51PM +0200, Petr Mladek wrote:
> __klp_disable_patch() should never be called when the patch is not
> enabled. Let's add the same warning that we have in __klp_enable_patch().
> 
> This allows to remove the check when calling klp_pre_unpatch_callback().
> It was strange anyway because it repeatedly checked per-patch flag
> for each patched object.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb134479c394..287f71e9dbfe 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -282,6 +282,9 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
>  
> +	if (WARN_ON(!patch->enabled))
> +		return -EINVAL;
> +
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> @@ -293,7 +296,7 @@ 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)
> +		if (obj->patched)
>  			klp_pre_unpatch_callback(obj);
>  
>  	/*
> -- 
> 1.8.5.6

Looks reasonable to me and cleans up the klp_pre_unpatch_callback()
calling condition.

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

Re: [PATCH 1/2] livepatch: Correctly call klp_post_unpatch_callback() in error paths

[Joe Lawrence]

On Fri, Oct 20, 2017 at 04:56:50PM +0200, Petr Mladek wrote:
>

Hi Petr,

Thank you for these fixups!  I ran them through the tests
in Documentation/livepatch/callbacks.txt just to be safe and it looks
good.  I wish I had added a few tests for klp_patch_object() failures,
but I would have needed to somehow force those errors.  At that point
I'd have more of a regression test than a human readable document.

Anyway, if this set can be squashed into for-4.15/callbacks, then great.
If not, then may I suggest a few rewordings/cleanups for this commit
message:

> The flag post_unpatch_enabled in struct klp_callbacks is need in
> the error paths. We need to call the post_unpatch callback
> only when the pre_patch one was called.
> 
> We should clear the flag in klp_post_unpatch_callback() to make
> sure that the callback is not called twice. It makes the API
> more safe.

The post_unpatch_enabled flag in struct klp_callbacks is set when a
pre-patch callback successfully executes, indicating that we need to
call a corresponding post-unpatch callback when the patch is reverted.
This is true for ordinary patch disable as well as the error paths of
klp_patch_object() callers.

> Note that we actually would call the callback twice in
> klp_module_coming() when klp_patch_object() fails.
> In this case, we explicitly call klp_post_unpatch_callback()
> for the failed object. And we are going to call it once
> again when reverting operations for all the patches by
> reusing the klp_module_going() code. There is a patch
> doing this in the queue.

As currently coded, we inadvertently execute the post-patch callback
twice in klp_module_coming() when klp_patch_object() fails:

  - We explicitly call klp_post_unpatch_callback() for the failed object
  - We call it again for the same object (and all the others) via
    klp_cleanup_module_patches_limited()

We should clear the flag in klp_post_unpatch_callback() to make
sure that the callback is not called twice. It makes the API
more safe.

(We could have removed the callback from the former error path as it
would be covered by the latter call, but I think that is is cleaner to
clear the post_unpatch_enabled after its invoked. For example, someone
might later decide to call the callback only when obj->patched flag is
set.)

> There was another mistake in the error path in klp_comming_module().
> It called klp_post_unpatch_callback() only when
> patch != klp_transition_patch. But klp_pre_patch_callback()
> was called even for this patch.

There is another mistake in the error path of klp_coming_module() in
which it skips the post-unpatch callback for the klp_transition_patch.
However, the pre-patch callback was called even for this patch, so be
sure to make the corresponding callbacks for all patches.
 
> In fact, we could remove klp_post_unpatch_callback() from
> the error path at all because it will be covered by
> the reuse of the klp_module_going() code. But I think
> that it is cleaner this way. For example, someone
> might later decide to call the callback only when
> obj->patched flag is set.

JL: (Moved up above.)

> 
> Finally, I used this opportunity to make klp_pre_patch_callback()
> more readable.

JL: Agreed that this is clearer to read, thanks. 

> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

> ---
>  kernel/livepatch/core.c | 4 +---
>  kernel/livepatch/core.h | 8 +++++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index cafb5a84417d..eb134479c394 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -894,9 +894,7 @@ int klp_module_coming(struct module *mod)
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
>  					patch->mod->name, obj->mod->name, ret);
>  
> -				if (patch != klp_transition_patch)
> -					klp_post_unpatch_callback(obj);
> -
> +				klp_post_unpatch_callback(obj);
>  				goto err;
>  			}
>  
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 6fc907b54e71..cc3aa708e0b4 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -12,10 +12,10 @@ static inline bool klp_is_object_loaded(struct klp_object *obj)
>  
>  static inline int klp_pre_patch_callback(struct klp_object *obj)
>  {
> -	int ret;
> +	int ret = 0;
>  
> -	ret = (obj->callbacks.pre_patch) ?
> -		(*obj->callbacks.pre_patch)(obj) : 0;
> +	if (obj->callbacks.pre_patch)
> +		ret = (*obj->callbacks.pre_patch)(obj);
>  
>  	obj->callbacks.post_unpatch_enabled = !ret;
>  
> @@ -39,6 +39,8 @@ static inline void klp_post_unpatch_callback(struct klp_object *obj)
>  	if (obj->callbacks.post_unpatch_enabled &&
>  	    obj->callbacks.post_unpatch)
>  		(*obj->callbacks.post_unpatch)(obj);
> +
> +	obj->callbacks.post_unpatch_enabled = false;
>  }
>  
>  #endif /* _LIVEPATCH_CORE_H */
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] livepatch: Additional fixes for callbacks feature

[Jiri Kosina]

On Fri, 20 Oct 2017, Petr Mladek wrote:

> I have found few small problems when reviewing the patch adding
> (un)patch callback, see
> https://lkml.kernel.org/r/1507921723-1996-2-git-send-email-joe.lawrence@redhat.com
> Feel free to merge them with the original patch if still possible.
> 
> Petr Mladek (2):
>   livepatch: Correctly call klp_post_unpatch_callback() in error paths
>   livepatch: __klp_disable_patch() should never be called for disabled
>     patches
> 
>  kernel/livepatch/core.c | 9 +++++----
>  kernel/livepatch/core.h | 8 +++++---
>  2 files changed, 10 insertions(+), 7 deletions(-)

Applied to for-4.15/callbacks (with Joe's changelog comments 
incorporated). Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] livepatch: __klp_disable_patch() should never be called for disabled patches

[Miroslav Benes]

On Fri, 20 Oct 2017, Petr Mladek wrote:

> __klp_disable_patch() should never be called when the patch is not
> enabled. Let's add the same warning that we have in __klp_enable_patch().
> 
> This allows to remove the check when calling klp_pre_unpatch_callback().
> It was strange anyway because it repeatedly checked per-patch flag
> for each patched object.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Heh, this is not a new thing. See 
http://lkml.kernel.org/r/20141128170735.GD32376@dhcp128.suse.cz

I don't know why it got lost. Those were crazy times...

Miroslav

> ---
>  kernel/livepatch/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb134479c394..287f71e9dbfe 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -282,6 +282,9 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
>  
> +	if (WARN_ON(!patch->enabled))
> +		return -EINVAL;
> +
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> @@ -293,7 +296,7 @@ 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)
> +		if (obj->patched)
>  			klp_pre_unpatch_callback(obj);
>  
>  	/*
> -- 
> 1.8.5.6
>