DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 06B8DC0587E8
Date: Tue, 29 Aug 2017 14:59:12 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jessica Yu <jeyu@kernel.org>, Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks
Message-ID: <20170829195912.m7wdoymu7fjmoioi@treble>
References: <1503688202-12121-1-git-send-email-joe.lawrence@redhat.com>
 <1503688202-12121-2-git-send-email-joe.lawrence@redhat.com>
 <20170829154902.sw7r4k3ofo7hv4ev@treble>
 <d504a1d1-3082-9428-c613-4b5b51a65ce9@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <d504a1d1-3082-9428-c613-4b5b51a65ce9@redhat.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5396
Lines: 121

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