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

[Josh Poimboeuf]

On Mon, Aug 14, 2017 at 10:53:08AM -0400, Joe Lawrence wrote:
> On 08/11/2017 04:44 PM, Josh Poimboeuf wrote:
> > On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
> >> +++ b/Documentation/livepatch/callbacks.txt
> >> @@ -0,0 +1,75 @@
> >> +(Un)patching Callbacks
> >> +======================
> >> +
> >> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> >> +to execute callback functions when a kernel object is (un)patched.
> > 
> > I think it would be helpful to put a little blurb here about why
> > callbacks are needed and when they might be used.  Maybe steal some of
> > the description from the first two bullet points here:
> > 
> >   https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble
> 
> Ok -- btw, can you explain this point: "patching otherwise unpatchable
> code (i.e., assembly)".  I wasn't sure if you were referring to the
> actual code, or modifying the machine state as setup by some init time
> assembly.

I meant actually patching assembly code.

> > Also, I tested stop_machine() in the callbacks and it seemed to work
> > fine.  It might be worth mentioning in the docs that it's an option.
> 
> I'll file that under the "you better know what you're doing" section. :)
> If your test would be a better use-case example or sample module than
> what's currently in the patchset, feel free to send it over and I can
> incorporate it.

Well, it's questionable whether using stop_machine() is a good idea, and
it's one of those "use as a last resort" things, so maybe we don't need
to add it to the sample module.

> >> +These callbacks differ from existing kernel facilities:
> >> +
> >> +  - Module init/exit code doesn't run when disabling and re-enabling a
> >> +    patch.
> >> +
> >> +  - A module notifier can't stop the to-be-patched module from loading.
> >> +
> >> +Callbacks are part of the klp_object structure and their implementation
> >> +is specific to the given object.  Other livepatch objects may or may not
> >> +be patched, irrespective of the target klp_object's current state.
> >> +
> >> +Callbacks can be registered for the following livepatch actions:
> >> +
> >> +  * Pre-patch    - before klp_object is patched
> >> +
> >> +  * Post-patch   - after klp_object has been patched and is active
> >> +                   across all tasks
> >> +
> >> +  * Pre-unpatch  - before klp_object is unpatched, patched code is active
> >> +
> >> +  * Post-unpatch - after klp_object has been patched, all code has been
> >> +		   restored and no tasks are running patched code
> >> +
> >> +Callbacks are only executed if its host klp_object is loaded.  For
> > 
> > "Callbacks are" -> "A callback is" ?
> 
> Okay.  What about the preceding plural-case instances?

I think it doesn't matter much, as long as each sentence is
grammatically consistent with itself.

> >> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> >> +{
> >> +	if (!obj->patched && obj->callbacks.pre_patch)
> >> +		return (*obj->callbacks.pre_patch)(obj);
> >> +	return 0;
> >> +}
> >> +static inline void klp_post_patch_callback(struct klp_object *obj)
> >> +{
> >> +	if (obj->patched && obj->callbacks.post_patch)
> >> +		(*obj->callbacks.post_patch)(obj);
> >> +}
> >> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> >> +{
> >> +	if (obj->patched && obj->callbacks.pre_unpatch)
> >> +		(*obj->callbacks.pre_unpatch)(obj);
> >> +}
> >> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> >> +{
> >> +	if (!obj->patched && obj->callbacks.post_unpatch)
> >> +		(*obj->callbacks.post_unpatch)(obj);
> >> +}
> >> +
> > 
> > Do these need the obj->patched checks?  As far as I can tell they seem
> > to be called in the right places and the checks are superfluous.
> 
> That is correct.  I can leave them (defensive coding) or take them out
> and perhaps add comments above to explain their use and assumptions.

Personally I'd rather get rid of the checks as I found them confusing.

If we really wanted to get defensive, we could add some WARNs, but that
might be overkill.

> >> --- a/kernel/livepatch/transition.c
> >> +++ b/kernel/livepatch/transition.c
> >> @@ -109,9 +109,6 @@ static void klp_complete_transition(void)
> >>  		}
> >>  	}
> >>  
> >> -	if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> >> -		module_put(klp_transition_patch->mod);
> >> -
> >>  	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> >>  	if (klp_target_state == KLP_PATCHED)
> >>  		klp_synchronize_transition();
> >> @@ -130,6 +127,22 @@ static void klp_complete_transition(void)
> >>  	}
> >>  
> >>  done:
> >> +	klp_for_each_object(klp_transition_patch, obj) {
> >> +		if (klp_target_state == KLP_PATCHED)
> >> +			klp_post_patch_callback(obj);
> >> +		else if (klp_target_state == KLP_PATCHED)
> > 
> > s/KLP_PATCHED/KLP_UNPATCHED
> 
> Ahh, I was so focused on the loadable module cases in
> module_coming/going that I botched this case.  Will fix for v3.
> 
> >> +			klp_post_unpatch_callback(obj);
> >> +	}
> >> +
> >> +	/*
> >> +	 * See complementary comment in __klp_enable_patch() for why we
> >> +	 * keep the module reference for immediate patches.
> >> +	 */
> >> +	if (!klp_transition_patch->immediate) {
> >> +		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> >> +			module_put(klp_transition_patch->mod);
> >> +	}
> >> +
> > 
> > Maybe combine these into a single 'if' for clarity:
> > 
> > 	if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
> > 	    !klp_transition_patch->immediate)
> > 		module_put(klp_transition_patch->mod);
> 
> How about this arrangement:
> 
> if (!klp_transition_patch->immediate &&
>     klp_target_state == KLP_UNPATCHED && !immediate_func) {
> 	module_put(klp_transition_patch->mod);
> }
> 
> 1) It leads with the klp_transition_patch->immediate variable, which the
> preceding comment and goto is all about and 2) brackets the multiline
> conditional part -- a personal preference, but I could drop for
> convention sake.

The bracket's fine with me.  Personally I think it makes more sense to
bunch the immediate checks together.

> >> + * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch
> >> + *       callback return status.  Try setting up a non-zero status
> >> + *       such as -19 (-ENODEV):
> >> + *
> >> + *       # Load demo livepatch, vmlinux is patched
> >> + *       insmod samples/livepatch/livepatch-callbacks-demo.ko
> >> + *
> >> + *       # Setup next pre-patch callback to return -ENODEV
> >> + *       echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret 
> > 
> > Git complained about trailing whitespace here ^
> > 
> >> + *
> >> + *       # Module loader refuses to load the target module
> >> + *       insmod samples/livepatch/livepatch-callbacks-mod.ko 
> > 
> > and here ^
> 
> Oh hey, look who was too cool to run checkpatch, again.
> 
> >> +/* Executed on object unpatching (ie, patch disablement) */
> >> +static void post_patch_callback(struct klp_object *obj)
> > 
> > s/unpatching/patching/
> > 
> 
> Good catch.
> 
> So v2 was a bit rushed to try and get something out there to talk about:
> 
> Are the callback locations accurate to your v1 suggestions?

Yeah, it all looked good to me.

> How do you feel about a pre-patch callback potentially preventing the
> loading of a kernel module -or- the patch module itself depending on
> which is loaded first?

I think that makes sense.

> Is the pre-patch return status sufficient? (ie, I couldn't see how
> post-patch, pre-unpatch, post-patch callbacks could affect the actions
> already set in motion.)

I think so.

-- 
Josh