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

[Joe Lawrence]

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