DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 049656D70E
Date: Wed, 16 Aug 2017 15:20:32 -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 v3] add (un)patch callbacks
Message-ID: <20170816202032.lpmi2ucklnq5gzqv@treble>
References: <1502911024-16143-1-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1502911024-16143-1-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: 2553
Lines: 61

On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> v3:
> 
> - livepatch.h
>   - drop obj->patched checks from pre/post-(un)patch funcs,
>     add preceding comment and note about obj->patched assumptions
>   - move core.c :: klp_is_module() to here
> 
> - klp_complete_transition()
>   - fix "else if (klp_target_state == KLP_UNPATCHED)" case
>   - combine conditional syntax when avoiding module_put for immediate
>     patches
>   - add check for klp_is_object_loaded to avoid callbacks for any
>     unloaded modules (necessary after removing obj->patched checks in
>     livepatch.h)
> 
> - Documentation
>   - added Josh's use-cases blurb in intro
>   - s/Callbacks are only executed/A callbacks is only executed/
> 
> - livepatch-callbacks-demo.c
>   - whitespace cleanup
> 
> I also wrote a quick test script (see below) to exercise some of the
> load/unload/enable/disable/error status combinations.  I'm not sure
> about some of the behaviors, most notably test6 with regard to
> post-unpatch-callbacks as executed on a cancelled transition.  (See
> results and comments further below.)

Yeah, that doesn't seem right.  Maybe in case of a pre-patch callback
error, we should only call post-unpatch callbacks for those objects
whose pre-patch callbacks were successfully called (and returned zero).
That would mean tracking on a per-object basis which objects had their
pre-patch callbacks called (successfully).

That would give the patch module a post-unpatch chance to tear down
anything it had set up in the pre-patch callback.

And the behavior should be documented.

> Also, maybe it's just my reading of the log, but would it be clearer if
> the "(un)patching ... complete" messages indicated that they are
> referring to a transaction?  It's a bit confusing to see "unpatching ...
> complete" before the pre-unpatch-callbacks ever execute.  Not a big
> deal, but I can send a follow up patch if others agree.

Hm.  I'm thinking this highlights the fact that the pre-unpatch callback
is being called in the wrong place.  It should actually be called before
the unpatching transition starts.  When called from
klp_unpatch_objects(), the new code is no longer running, so it's
effectively post-patch instead of pre-patch.

Another random thought: maybe we should show the "patching complete"
message *after* the post-patch callback is run.  That would be more
honest with the user, as technically, the post-patch callback is part of
the patching process.

And a similar comment for the "unpatching complete" message.

-- 
Josh