DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0E71880C03
Date: Wed, 19 Jul 2017 15:49:52 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Joe Lawrence <joe.lawrence@redhat.com>, 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>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH] livepatch: add (un)patch hooks
Message-ID: <20170719204952.4fyhtig3rbw7z4w4@treble>
References: <1499868600-10176-1-git-send-email-joe.lawrence@redhat.com>
 <1499868600-10176-2-git-send-email-joe.lawrence@redhat.com>
 <20170717155144.GF32632@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170717155144.GF32632@pathway.suse.cz>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4736
Lines: 121

On Mon, Jul 17, 2017 at 05:51:44PM +0200, Petr Mladek wrote:
> On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> > When the livepatch core executes klp_(un)patch_object, call out to a
> > livepatch-module specified array of callback hooks.  These hooks provide
> > a notification mechanism for livepatch modules when klp_objects are
> > (un)patching. This may be most interesting when another kernel module
> > is a klp_object target and the livepatch module needs to execute code
> > after the target is loaded, but before its module_init code is run.
> > 
> > The patch-hook executes right before patching objects and the
> > unpatch-hook executes right after unpatching objects.
> > 
> > diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
> > new file mode 100644
> > index 000000000000..ef18101a3b90
> > --- /dev/null
> > +++ b/Documentation/livepatch/hooks.txt
> > @@ -0,0 +1,98 @@
> > +(Un)patching Hooks
> > +==================
> > +
> > +Livepatching (un)patch-hooks provide a mechanism to register and execute
> > +a set of callback functions when the kernel's livepatching core performs
> > +an (un)patching operation on a given kernel object.
> 
> The above is correct but it is a bit hard to understand what it really
> means. Josh's discussion about the naming suggests that I am not the only
> one who is confused ;-)
> 
> We need to make it clear that there are 4 basic situations
> where these hooks are called:
> 
>   + patch hook is called when:
> 
> 	1. livepatch is being enabled and object is loaded
> 	2. livepatch is enabled and object is being loaded

These could be stated much simpler:

	1. the object is about to be patched

>   + unpatch hook is called when
> 
> 	3. livepatch is enabled and object is being removed
> 	4. livepatch is being disabled and object is loaded

And this one too:

	2. the object was just unpatched

The rest are just unnecessary details IMO.  When writing a hook for a
particular object, the patch author shouldn't need to care about the
other objects and whether they're patched or unpatched.

Maybe we just need a warning/reminder in the documentation that this
hook is specific to the given object, and other objects could be patched
or unpatched irrespective of the target object's state.

> Note that this document mostly talks only about the two situations
> when the livepatch is enabled and the patched object is being
> loaded or removed.
> 
> But it is still quite tricky to understand what can be modified
> a safe way. We need to be careful about different things
> in the different situations.
> 
> If the patched object is beeing added/removed, we know that its
> code is not being used but the code from the rest of the patch
> is already in use. The module is not yet or not longer properly
> initialized. Therefore it might be too early or too late to
> register or unregister any of its services in the rest of
> the system. Basically it limits the changes only to
> to the object (module) itself.
> 
> If the patch is being enabled, it is another story. The object
> is already initialized and its old code is used but the new
> code from the patch is not yet or not longer used. It suggests
> that it might be safe to do some changes related to the
> new code in the patch. But we need to be careful because
> the system is using the old code.
> 
> 
> But there are actually 4 more situations. If we use the consistency
> model, different parts of the system might use different code.
> I mean that:
> 
>   + patch hook is called also when:
> 
>      + livepatch is being enabled and object is being loaded
>      + livepatch is being disabled and object is being loaded
> 
>   + unpatch hook is called when:
> 
>      + livepatch is being enabled and object is being removed
>      + livepatch is being disabled and object is being removed
> 
> 
> It is a bit easier if you run the hook for vmlinux
> because it is always running.

Again I think this is all overthinking it.  The patch hook should be
specific to the object.  It shouldn't make assumptions about other
objects.

> I am sorry for the long mail. But I have really troubles to
> understand and describe what can be done with these hooks
> a safe way.
> 
> It might help if you share some real-life examples.

Agreed, we should share some real world examples.  For a few cases, load
hooks were extremely useful.  But most of our experience has been with
the kpatch consistency model, so we need to revisit our past findings
and view them through the livepatch lens.

One crazy -- but potentially very useful -- idea would be if the user
were allowed to run stop_machine() from the load hook.  If possible,
that would help prevent a lot of race conditions.

-- 
Josh