lkml.org 
[lkml]   [2017]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2017-08-30 15:27    [W:0.085 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site