DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9E820285B9
Date: Thu, 13 Jul 2017 20:46:40 -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] livepatch: add (un)patch hooks
Message-ID: <20170714014640.nhoowbrleu6kdka2@treble>
References: <1499868600-10176-1-git-send-email-joe.lawrence@redhat.com>
 <1499868600-10176-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: <1499868600-10176-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: 1886
Lines: 41

On Wed, Jul 12, 2017 at 10:10:00AM -0400, 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.

And it's also useful for vmlinux.  Patch module load/unload is separate
from enable/disable, so the module init/exit functions can't be used for
patch-specific changes (e.g., global data changes).

> The patch-hook executes right before patching objects and the
> unpatch-hook executes right after unpatching objects.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Thanks for posting it.  We found this to be a useful feature in the
past, not quite as useful as shadow data, but still good to have for
certain cases.

Instead of "load hooks" I think it would be more accurate to call them
"enable/disable hooks".  (Maybe "callbacks" would be better than
"hooks"?  Not sure...)

Even better, we might want to be specific: "pre enable hooks" and "post
disable hooks".  (Or "pre patch hooks" and "post unpatch hooks"?)
Because we might eventually decide we need the corresponding "post
enable hooks" and "pre disable hooks" as well.

For the enable case, I think it would be a nice feature if we checked
the return code and aborted the patching operation on error.  I think
that should be easy enough.

For the unload case, it's too late to do anything, so I'd say a void
return code would be better.  Otherwise it implies that we actually do
something about it.  Maybe in that case we can leave it up to the user
to decide whether to print an error or WARN() or whatever.

-- 
Josh