Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720368 > unrolled thread

[PATCH v4 0/3] livepatch callbacks

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] livepatch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-25 21:20 +0200
    [PATCH v4 2/3] livepatch: move transition "complete" notice into klp_complete_transition() Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-25 21:20 +0200
      Re: [PATCH v4 2/3] livepatch: move transition "complete" notice into  klp_complete_transition() Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-29 18:00 +0200
    [PATCH v4 3/3] livepatch: add transition notices Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-25 21:20 +0200
      Re: [PATCH v4 3/3] livepatch: add transition notices Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-29 18:00 +0200
    Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-29 17:50 +0200
      Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-29 21:30 +0200
        Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-29 22:00 +0200
          Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-30 15:30 +0200
            Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-30 16:30 +0200
    Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-30 16:50 +0200

#1720368 — [PATCH v4 0/3] livepatch callbacks

v4:

- Move callback helpers into core.h

- Move klp_pre_patch_callback() and klp_pre_unpatch_callback()
  invocations into __klp_enable_patch() and __klp_disable_patch()

  - klp_patch_object() and klp_unpatch_objects()
    - Do not run pre-unpatch callbacks from here

- Add a pre_patch_status member to klp_object so when a pre-patch
  callback fails, the helpers can skip any post-patch, pre-unpatch,
  post-unpatch callbacks

- klp_module_coming() and klp_module_going()
  - Do not run post-patch or pre-unpatch callbacks for current
    klp_transition_patch

- Documentation
  - Add various test cases and provide commentary

- Samples
  - Create two target modules: a simple one and another that invokes a
    worker function that sleeps for a long time

- Added two follow-up patches:

  - livepatch: move transition "complete" notice into
    klp_complete_transition() - this pushes the "patching complete" message
    after the post-patch callbacks

  - livepatch: add transition notices - these were helpful during
    debugging of the callback patch.  The transaction annotations were
    also used in the Documentation file tese cases to illustrate the
    order of operations.

  Note that these two patches could be standalone, I include them here
  in this patchset since they affect the content/ordering of kernel logs
  that were included as part of the Documentation.

Joe Lawrence (3):
  livepatch: add (un)patch callbacks
  livepatch: move transition "complete" notice into
    klp_complete_transition()
  livepatch: add transition notices

 Documentation/livepatch/callbacks.txt           | 595 ++++++++++++++++++++++++
 include/linux/livepatch.h                       |  18 +
 kernel/livepatch/core.c                         |  56 ++-
 kernel/livepatch/core.h                         |  78 ++++
 kernel/livepatch/patch.c                        |   1 +
 kernel/livepatch/transition.c                   |  45 +-
 samples/livepatch/Makefile                      |   3 +
 samples/livepatch/livepatch-callbacks-busymod.c |  72 +++
 samples/livepatch/livepatch-callbacks-demo.c    | 234 ++++++++++
 samples/livepatch/livepatch-callbacks-mod.c     |  55 +++
 10 files changed, 1140 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/livepatch/callbacks.txt
 create mode 100644 samples/livepatch/livepatch-callbacks-busymod.c
 create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
 create mode 100644 samples/livepatch/livepatch-callbacks-mod.c

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1720369 — [PATCH v4 2/3] livepatch: move transition "complete" notice into klp_complete_transition()

klp_complete_transition() performs a bit of housework before a
transition to KLP_PATCHED or KLP_UNPATCHED is actually completed
(including post-(un)patch callbacks).  To be consistent, move the
transition "complete" kernel log notice out of
klp_try_complete_transition() and into klp_complete_transition().

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/livepatch/transition.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7bf55b7f3687..53887f0bca10 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -136,6 +136,9 @@ static void klp_complete_transition(void)
 			klp_post_unpatch_callback(obj);
 	}
 
+	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
+		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
 	/*
 	 * See complementary comment in __klp_enable_patch() for why we
 	 * keep the module reference for immediate patches.
@@ -423,9 +426,6 @@ void klp_try_complete_transition(void)
 	}
 
 success:
-	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
-		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
-
 	/* we're done, now cleanup the data structures */
 	klp_complete_transition();
 }
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1722583 — Re: [PATCH v4 2/3] livepatch: move transition "complete" notice into klp_complete_transition()

On Fri, Aug 25, 2017 at 03:10:01PM -0400, Joe Lawrence wrote:
> klp_complete_transition() performs a bit of housework before a
> transition to KLP_PATCHED or KLP_UNPATCHED is actually completed
> (including post-(un)patch callbacks).  To be consistent, move the
> transition "complete" kernel log notice out of
> klp_try_complete_transition() and into klp_complete_transition().
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

I think the Signed-off-by should always be the last line.  Otherwise:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1720373 — [PATCH v4 3/3] livepatch: add transition notices

Log a few kernel debug messages at the beginning of the following livepatch
transition functions:

  klp_complete_transition()
  klp_cancel_transition()
  klp_init_transition()
  klp_reverse_transition()

Also update the log notice message in klp_start_transition() for similar
verbiage as the above messages.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/livepatch/transition.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 53887f0bca10..3d44a3cf27be 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -82,6 +82,10 @@ static void klp_complete_transition(void)
 	unsigned int cpu;
 	bool immediate_func = false;
 
+	pr_debug("'%s': completing %s transition\n",
+		 klp_transition_patch->mod->name,
+		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*
 		 * All tasks have transitioned to KLP_UNPATCHED so we can now
@@ -163,6 +167,9 @@ void klp_cancel_transition(void)
 	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
 		return;
 
+	pr_debug("'%s': canceling transition, unpatching\n",
+		 klp_transition_patch->mod->name);
+
 	klp_target_state = KLP_UNPATCHED;
 	klp_complete_transition();
 }
@@ -441,7 +448,8 @@ void klp_start_transition(void)
 
 	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
 
-	pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+	pr_notice("'%s': starting %s transition\n",
+		  klp_transition_patch->mod->name,
 		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
 
 	/*
@@ -489,6 +497,9 @@ void klp_init_transition(struct klp_patch *patch, int state)
 
 	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
 
+	pr_debug("'%s': initializing %s transition\n", patch->mod->name,
+		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
 	klp_transition_patch = patch;
 
 	/*
@@ -562,6 +573,11 @@ void klp_reverse_transition(void)
 	unsigned int cpu;
 	struct task_struct *g, *task;
 
+	pr_debug("'%s': reversing transition from %s\n",
+		 klp_transition_patch->mod->name,
+		 klp_target_state == KLP_PATCHED ? "patching to unpatching" :
+						   "unpatching to patching");
+
 	klp_transition_patch->enabled = !klp_transition_patch->enabled;
 
 	klp_target_state = !klp_target_state;
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1722577 — Re: [PATCH v4 3/3] livepatch: add transition notices

On Fri, Aug 25, 2017 at 03:10:02PM -0400, Joe Lawrence wrote:
> Log a few kernel debug messages at the beginning of the following livepatch
> transition functions:
> 
>   klp_complete_transition()
>   klp_cancel_transition()
>   klp_init_transition()
>   klp_reverse_transition()
> 
> Also update the log notice message in klp_start_transition() for similar
> verbiage as the above messages.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

Same Signed-off-by comment here.  Otherwise:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1722572 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> +Test 6
> +------
> +
> +Test a scenario where a vmlinux pre-patch callback returns a non-zero
> +status (ie, failure):
> +
> +- load target module
> +- load livepatch -ENODEV
> +- unload target module
> +
> +First load a target module:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
> +  [   80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> +
> +Load the livepatch module, setting its 'pre_patch_ret' value to -19
> +(-ENODEV).  When its vmlinux pre-patch callback executed, this status
> +code will propagate back to the module-loading subsystem.  The result is
> +that the insmod command refuses to load the livepatch module:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
> +  [   82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
> +  [   82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> +  [   82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> +  [   82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
> +  [   82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
> +  [   82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
> +  [   82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> +  [   82.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> +  [   82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
> +  [   82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
> +
> +  % rmmod samples/livepatch/livepatch-callbacks-mod.ko
> +  [   84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

First off, this documentation is very nice because it clarifies all the
callback scenarios and edge cases.

The above situation still seems a little odd to me.  If I understand
correctly, the target module was never patched, and its pre_patch
callback was never called.  But its post_unpatch callback *was* called.
That doesn't seem right.

Maybe we should change the condition a little bit.  Currently it's:

  No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
  for a given klp_object if its pre-patch callback returned non-zero
  status.

I think that might have been my idea, but seeing the above case makes it
clear that it's not quite right.  Maybe it should instead be:

  No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
  for a given klp_object if the object failed to patch, due to a failed
  pre_patch callback or for any other reason.

  If the object did successfully patch, but the patch transition never
  started for some reason (e.g., if another object failed to patch),
  only the post-unpatch callback will be called.

So then, instead of tracking whether the pre-patch callback succeeded,
we just need to track whether the object was patched (which we already
do, with obj->patched).

What do you think?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1722737 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
>> +Test 6
>> +------
>> +
>> +Test a scenario where a vmlinux pre-patch callback returns a non-zero
>> +status (ie, failure):
>> +
>> +- load target module
>> +- load livepatch -ENODEV
>> +- unload target module
>> +
>> +First load a target module:
>> +
>> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
>> +  [   80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
>> +
>> +Load the livepatch module, setting its 'pre_patch_ret' value to -19
>> +(-ENODEV).  When its vmlinux pre-patch callback executed, this status
>> +code will propagate back to the module-loading subsystem.  The result is
>> +that the insmod command refuses to load the livepatch module:
>> +
>> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
>> +  [   82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
>> +  [   82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
>> +  [   82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
>> +  [   82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
>> +  [   82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
>> +  [   82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
>> +  [   82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
>> +  [   82.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
>> +  [   82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
>> +  [   82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
>> +
>> +  % rmmod samples/livepatch/livepatch-callbacks-mod.ko
>> +  [   84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
> 
> First off, this documentation is very nice because it clarifies all the
> callback scenarios and edge cases.
> 
> The above situation still seems a little odd to me.  If I understand
> correctly, the target module was never patched, and its pre_patch
> callback was never called.  But its post_unpatch callback *was* called.
> That doesn't seem right.

Ah, this does look to be a bug.

> Maybe we should change the condition a little bit.  Currently it's:
> 
>   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
>   for a given klp_object if its pre-patch callback returned non-zero
>   status.
> 
> I think that might have been my idea, but seeing the above case makes it
> clear that it's not quite right.

It could have been correct if the code differentiated between a
never-run pre_patch_status of 0 (by kzalloc) and a successful
pre_patch_status of 0 (by callback return), I think.

>                                   Maybe it should instead be:
> 
>   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
>   for a given klp_object if the object failed to patch, due to a failed
>   pre_patch callback or for any other reason.
> 
>   If the object did successfully patch, but the patch transition never
>   started for some reason (e.g., if another object failed to patch),
>   only the post-unpatch callback will be called.

That description sounds correct...

> So then, instead of tracking whether the pre-patch callback succeeded,
> we just need to track whether the object was patched (which we already
> do, with obj->patched).
> 
> What do you think?

I think this would only work if there was a sticky
"obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
the very end of klp_complete_transition()... by that point, obj->patched
will have already been cleared by klp_unpatch_objects.

We could maybe move obj->patched assignments out to encapsulate the pre
and post callbacks... but I would need to think about that a while.  It
seems pretty clear and symmetric as it is today (immediately set in
klp_(un)patch_object().

Perhaps a more careful checking of obj->pre_patch_callback_status is all
we need?  (I can't think of anything more succinct than adding a
obj->pre_patch_callback_done variable to the mix.)

-- Joe

[toc] | [prev] | [next] | [standalone]

#1722792 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Tue, Aug 29, 2017 at 03:22:06PM -0400, Joe Lawrence wrote:
> On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> > On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> >> +Test 6
> >> +------
> >> +
> >> +Test a scenario where a vmlinux pre-patch callback returns a non-zero
> >> +status (ie, failure):
> >> +
> >> +- load target module
> >> +- load livepatch -ENODEV
> >> +- unload target module
> >> +
> >> +First load a target module:
> >> +
> >> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
> >> +  [   80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> >> +
> >> +Load the livepatch module, setting its 'pre_patch_ret' value to -19
> >> +(-ENODEV).  When its vmlinux pre-patch callback executed, this status
> >> +code will propagate back to the module-loading subsystem.  The result is
> >> +that the insmod command refuses to load the livepatch module:
> >> +
> >> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
> >> +  [   82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
> >> +  [   82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> >> +  [   82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> >> +  [   82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
> >> +  [   82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
> >> +  [   82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
> >> +  [   82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> >> +  [   82.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> >> +  [   82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
> >> +  [   82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
> >> +
> >> +  % rmmod samples/livepatch/livepatch-callbacks-mod.ko
> >> +  [   84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
> > 
> > First off, this documentation is very nice because it clarifies all the
> > callback scenarios and edge cases.
> > 
> > The above situation still seems a little odd to me.  If I understand
> > correctly, the target module was never patched, and its pre_patch
> > callback was never called.  But its post_unpatch callback *was* called.
> > That doesn't seem right.
> 
> Ah, this does look to be a bug.
> 
> > Maybe we should change the condition a little bit.  Currently it's:
> > 
> >   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> >   for a given klp_object if its pre-patch callback returned non-zero
> >   status.
> > 
> > I think that might have been my idea, but seeing the above case makes it
> > clear that it's not quite right.
> 
> It could have been correct if the code differentiated between a
> never-run pre_patch_status of 0 (by kzalloc) and a successful
> pre_patch_status of 0 (by callback return), I think.
> 
> >                                   Maybe it should instead be:
> > 
> >   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> >   for a given klp_object if the object failed to patch, due to a failed
> >   pre_patch callback or for any other reason.
> > 
> >   If the object did successfully patch, but the patch transition never
> >   started for some reason (e.g., if another object failed to patch),
> >   only the post-unpatch callback will be called.
> 
> That description sounds correct...
> 
> > So then, instead of tracking whether the pre-patch callback succeeded,
> > we just need to track whether the object was patched (which we already
> > do, with obj->patched).
> > 
> > What do you think?
> 
> I think this would only work if there was a sticky
> "obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
> the very end of klp_complete_transition()... by that point, obj->patched
> will have already been cleared by klp_unpatch_objects.
> 
> We could maybe move obj->patched assignments out to encapsulate the pre
> and post callbacks... but I would need to think about that a while.  It
> seems pretty clear and symmetric as it is today (immediately set in
> klp_(un)patch_object().
> 
> Perhaps a more careful checking of obj->pre_patch_callback_status is all
> we need?  (I can't think of anything more succinct than adding a
> obj->pre_patch_callback_done variable to the mix.)

Makes sense.  I think you're right that obj->patched wouldn't work.

But there's one more weird case I didn't mention.  If the patch has a
post-unpatch callback, but it doesn't have a pre-patch callback, then
'obj->pre_patch_callback_done' would never get set and the post-unpatch
callback would never get called, even if the patch was successful.

So instead of 'obj->pre_patch_callback_done', how about
'obj->callbacks_enabled'?

It could be set in the following cases:

  a) if the object has a pre_patch callback, set obj->callbacks_enabled
     after the pre_patch callback succeeds; 

  b) else, if the patch does *not* have a pre_patch callback, set
     obj->callbacks_enabled after klp_patch_object() succeeds.

And the variable would need to be cleared after the post_unpatch
callback was run.

It's a bit complicated, but that seems to be the most logicial behavior
as far as I can tell.

Thoughts?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1723337 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Tue, Aug 29, 2017 at 02:59:12PM -0500, Josh Poimboeuf wrote:
> On Tue, Aug 29, 2017 at 03:22:06PM -0400, Joe Lawrence wrote:
> > On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> > > On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> > >> +Test 6
> > >> +------
> > >> +
> > >> +Test a scenario where a vmlinux pre-patch callback returns a non-zero
> > >> +status (ie, failure):
> > >> +
> > >> +- load target module
> > >> +- load livepatch -ENODEV
> > >> +- unload target module
> > >> +
> > >> +First load a target module:
> > >> +
> > >> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
> > >> +  [   80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> > >> +
> > >> +Load the livepatch module, setting its 'pre_patch_ret' value to -19
> > >> +(-ENODEV).  When its vmlinux pre-patch callback executed, this status
> > >> +code will propagate back to the module-loading subsystem.  The result is
> > >> +that the insmod command refuses to load the livepatch module:
> > >> +
> > >> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
> > >> +  [   82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
> > >> +  [   82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> > >> +  [   82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> > >> +  [   82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
> > >> +  [   82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
> > >> +  [   82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
> > >> +  [   82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> > >> +  [   82.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> > >> +  [   82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
> > >> +  [   82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
> > >> +
> > >> +  % rmmod samples/livepatch/livepatch-callbacks-mod.ko
> > >> +  [   84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
> > > 
> > > First off, this documentation is very nice because it clarifies all the
> > > callback scenarios and edge cases.
> > > 
> > > The above situation still seems a little odd to me.  If I understand
> > > correctly, the target module was never patched, and its pre_patch
> > > callback was never called.  But its post_unpatch callback *was* called.
> > > That doesn't seem right.
> > 
> > Ah, this does look to be a bug.
> > 
> > > Maybe we should change the condition a little bit.  Currently it's:
> > > 
> > >   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> > >   for a given klp_object if its pre-patch callback returned non-zero
> > >   status.
> > > 
> > > I think that might have been my idea, but seeing the above case makes it
> > > clear that it's not quite right.
> > 
> > It could have been correct if the code differentiated between a
> > never-run pre_patch_status of 0 (by kzalloc) and a successful
> > pre_patch_status of 0 (by callback return), I think.
> > 
> > >                                   Maybe it should instead be:
> > > 
> > >   No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> > >   for a given klp_object if the object failed to patch, due to a failed
> > >   pre_patch callback or for any other reason.
> > > 
> > >   If the object did successfully patch, but the patch transition never
> > >   started for some reason (e.g., if another object failed to patch),
> > >   only the post-unpatch callback will be called.
> > 
> > That description sounds correct...
> > 
> > > So then, instead of tracking whether the pre-patch callback succeeded,
> > > we just need to track whether the object was patched (which we already
> > > do, with obj->patched).
> > > 
> > > What do you think?
> > 
> > I think this would only work if there was a sticky
> > "obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
> > the very end of klp_complete_transition()... by that point, obj->patched
> > will have already been cleared by klp_unpatch_objects.
> > 
> > We could maybe move obj->patched assignments out to encapsulate the pre
> > and post callbacks... but I would need to think about that a while.  It
> > seems pretty clear and symmetric as it is today (immediately set in
> > klp_(un)patch_object().
> > 
> > Perhaps a more careful checking of obj->pre_patch_callback_status is all
> > we need?  (I can't think of anything more succinct than adding a
> > obj->pre_patch_callback_done variable to the mix.)
> 
> Makes sense.  I think you're right that obj->patched wouldn't work.
> 
> But there's one more weird case I didn't mention.  If the patch has a
> post-unpatch callback, but it doesn't have a pre-patch callback, then
> 'obj->pre_patch_callback_done' would never get set and the post-unpatch
> callback would never get called, even if the patch was successful.

Interesting case.  I didn't code anything up, but the idea was that the
other callbacks would only run iff pre_patch_done && status == 0 ||
!pre_patch_callback.  But the following suggestion is clearer IMHO ...

> So instead of 'obj->pre_patch_callback_done', how about
> 'obj->callbacks_enabled'?
> 
> It could be set in the following cases:
> 
>   a) if the object has a pre_patch callback, set obj->callbacks_enabled
>      after the pre_patch callback succeeds; 
> 
>   b) else, if the patch does *not* have a pre_patch callback, set
>      obj->callbacks_enabled after klp_patch_object() succeeds.
> 
> And the variable would need to be cleared after the post_unpatch
> callback was run.
> 
> It's a bit complicated, but that seems to be the most logicial behavior
> as far as I can tell.
> 
> Thoughts?

What if we flip it around as "callbacks_disabled"?  By default, kzalloc
would init as false.  It would only be set to true if the pre-patch
callback is provided and if it returns failure.  Would that reduce the
number of conditions when we need to set this var?

Also, as you noted, I think it would need to reset/cleared after the
post-patch callback.  (For the livepatch-already-loaded cases.)

-- Joe

[toc] | [prev] | [next] | [standalone]

#1723382 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Wed, Aug 30, 2017 at 09:27:16AM -0400, Joe Lawrence wrote:
> > So instead of 'obj->pre_patch_callback_done', how about
> > 'obj->callbacks_enabled'?
> > 
> > It could be set in the following cases:
> > 
> >   a) if the object has a pre_patch callback, set obj->callbacks_enabled
> >      after the pre_patch callback succeeds; 
> > 
> >   b) else, if the patch does *not* have a pre_patch callback, set
> >      obj->callbacks_enabled after klp_patch_object() succeeds.
> > 
> > And the variable would need to be cleared after the post_unpatch
> > callback was run.
> > 
> > It's a bit complicated, but that seems to be the most logicial behavior
> > as far as I can tell.
> > 
> > Thoughts?
> 
> What if we flip it around as "callbacks_disabled"?  By default, kzalloc
> would init as false.  It would only be set to true if the pre-patch
> callback is provided and if it returns failure.  Would that reduce the
> number of conditions when we need to set this var?

Yeah, 'callbacks_disabled' sounds better.

> Also, as you noted, I think it would need to reset/cleared after the
> post-patch callback.  (For the livepatch-already-loaded cases.)

Since it can only be set when the pre-patch fails, I think it would only
need to be cleared after post-unpatch?

Or another alternative would be to unconditionally clear and/or set it
in klp_pre_patch_callback() so that its previous value doesn't matter.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1723404 — Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> @@ -871,6 +882,13 @@ int klp_module_coming(struct module *mod)
>  			pr_notice("applying patch '%s' to loading module '%s'\n",
>  				  patch->mod->name, obj->mod->name);
>  
> +			ret = klp_pre_patch_callback(obj);
> +			if (ret) {
> +				pr_warn("pre-patch callback failed for object '%s'\n",
> +					obj->name);
> +				goto err;
> +			}
> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",

If klp_pre_patch_callback() succeeds but klp_patch_object() fails, the
post-unpatch callback needs to be called in the error path.

> +/**
> + * klp_pre_patch_callback - executed before klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.

Can this comment be removed since it no longer checks obj->patched?

> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> +	obj->pre_patch_callback_status =
> +		(obj->callbacks.pre_patch) ?
> +			(*obj->callbacks.pre_patch)(obj) : 0;
> +
> +	return obj->pre_patch_callback_status;
> +}
> +
> +/**
> + * klp_post_patch_callback() - executed after klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.

Ditto here and below.

> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.post_patch)
> +		(*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - executed before klp_object is unpatched
> + *                              and is active across all tasks
> + * @obj:	invoke callback for this klp_object
> + *
> + * This callback will not be run if the pre-patch callback status was
> + * non-zero.

I think this comment should be a little broader.  The callback won't be
called if the object fails to patch for *any* reason.

Also, I think all the comments about when the callbacks are run or not
run would be better placed in a paragraph above the 'struct
klp_callbacks' definition in include/linux/livepatch.h.

And actually, IMO, all the functions in core.h are straightforward
enough that they don't need *any* function header comments.  And the
same for klp_is_object_loaded().  I would just remove all the core.h
comments, and just add a short explanation in livepatch.h about when the
callbacks are called and not called.

> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> +	if (!obj->pre_patch_callback_status &&
> +	    obj->callbacks.pre_unpatch)
> +		(*obj->callbacks.pre_unpatch)(obj);
> +}

I think pre_patch_callback_status (or callbacks_disabled) doesn't need
to be checked here, right?  Since if we got this far, the patching was
successful and callbacks will be enabled by definition?

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web