DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 72626C047B88
Date: Tue, 29 Aug 2017 10:49:02 -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: <20170829154902.sw7r4k3ofo7hv4ev@treble>
References: <1503688202-12121-1-git-send-email-joe.lawrence@redhat.com>
 <1503688202-12121-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1503688202-12121-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3136
Lines: 69

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