Date: Thu, 20 Jul 2017 16:36:08 +0200
From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, 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: <20170720143608.GA13895@pathway.suse.cz>
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>
 <5ad1126c-98ba-29bb-12a9-388af09d0944@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5ad1126c-98ba-29bb-12a9-388af09d0944@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7330
Lines: 225

On Wed 2017-07-19 14:59:48, Joe Lawrence wrote:
> On 07/17/2017 11:51 AM, Petr Mladek wrote:
> > On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> > 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
> > 
> >   + unpatch hook is called when
> > 
> > 	3. livepatch is enabled and object is being removed
> > 	4. livepatch is being disabled and object is loaded
> > 
> If you look at it from the inside out, the concept can be boiled down to
> "kernel object patching notifiers".  Essentially whenever a klp_object
> is being patched or unpatched, the callbacks are executed.

I see your point. I somewhat got the impression that feature
description primary talked about situations when the affected
object/module was loaded later than the livepatch. We could
change things more easily in this situations. It rang the bells
that the hooks would run also in other situations where we
would need to take more care.

I do not know if it is my personal problem or we might just
need to improve the documentation a bit. I wonder if the
section "For module targets" consfused me. There are 16
lines about one situations and 3 lines about the other.
Also the indented lines are more eye catching than the
rest.


> however, I think a livepatch counterpart needs to consider a bit more:
> locking, unpatched/patched code access, etc.
> 
> > 
> >> +The hooks are provided and registered by a livepatch module as part of
> >> +klp_objects that make up its klp_patch structure.  Both patch and
> >> +unpatch-hook function signatures accept a pointer to a klp_object
> >> +argument and return an integer status, ie:
> > 
> > I would put this into separate section and make it clear
> > that it is a sample code.
> 
> It wasn't exactly /example/ code, it just seemed more succinct to
> provide some /pseudo/ code to explain the data structure relationships.
> 
> If it is redundant then I can remove from v2

I think that it is fine. It is much easier to understand that
hundred of words.

I do not know. Maybe, it was just mentioned to early. Maybe,
I expected more details about the feature usefulness before
"implementation" details.

Maybe we could forget about this problem. I wrote about it
because I got a bit confused when I read the patch for
the first time. But it is a minor problem.


> >> +  static int patch_hook(struct klp_object *obj)
> >> +  {
> >> +  	/* ... */
> >> +  }
> >> +  static int unpatch_hook(struct klp_object *obj)
> >> +  {
> >> +  	/* ... */
> >> +  }
> >> +
> >> +  static struct klp_hook patch_hooks[] = {
> >> +  	{
> >> +  		.hook = patch_hook,
> >> +  	}, { }
> >> +  };
> >> +  static struct klp_hook unpatch_hooks[] = {
> >> +  	{
> >> +  		.hook = unpatch_hook,
> >> +  	}, { }
> >> +  };
> >> +
> >> +  static struct klp_object objs[] = {
> >> +  	{
> >> +  		/* ... */
> >> +  		.patch_hooks = patch_hooks,
> >> +  		.unpatch_hooks = unpatch_hooks,
> >> +  	}, { }
> >> +  };
> >> +
> >> +  static struct klp_patch patch = {
> >> +  	.mod = THIS_MODULE,
> >> +  	.objs = objs,
> >> +  };
> >> +
> >> +If a hook returns non-zero status, the livepatching core will log a
> >> +hook failure warning message.
> > 
> >> +Multiple (un)patch-hooks may be registered per klp_object.  Each hook
> >> +will execute regardless of any previously executed hook's non-zero
> >> +return status.
> > 
> > We should pass the error down the stack. If will prevent either the
> > patch or the patched module of being loaded. Of course, we could
> > not do much if the patch or the patched object is being removed.
> 
> I know Josh and Miroslav pointed this out elsewhere in the thread,
> but I don't know how I feel about using the hooks to affect the
> patch(ed) module loading.  Making these "void" feels simpler IMHO.

I think that the system safety is much more important than a
simplicity.

People use livepatches in situatution when the system restart is
too expensive. If we detect a problem and could stop the operation
before it causes some damage, it is very valuable. IMHO, this is
the case here.


> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >> index b9628e43c78f..ff3685470057 100644
> >> --- a/kernel/livepatch/core.c
> >> +++ b/kernel/livepatch/core.c
> >> @@ -49,11 +49,6 @@
> >>  
> >>  static struct kobject *klp_root_kobj;
> >>  
> >> -static bool klp_is_module(struct klp_object *obj)
> >> -{
> >> -	return obj->name;
> >> -}
> >> -
> >>  static bool klp_is_object_loaded(struct klp_object *obj)
> >>  {
> >>  	return !obj->name || obj->mod;
> >> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> >> index 52c4e907c14b..c8084a18ddb7 100644
> >> --- a/kernel/livepatch/patch.c
> >> +++ b/kernel/livepatch/patch.c
> >> @@ -235,25 +235,60 @@ static int klp_patch_func(struct klp_func *func)
> >>  	return ret;
> >>  }
> >>  
> >> +/**
> >> + * klp_run_hook - execute a given klp_hook callback
> >> + * @hook:	callback hook
> >> + * @obj:	kernel object that has been hooked
> >> + *
> >> + * Return: return value from hook, or 0 if none is currently associated
> >> + */
> >> +static int klp_run_hook(struct klp_hook *hook, struct klp_object *obj)
> >> +{
> >> +	if (hook && hook->hook)
> >> +		return (*hook->hook)(obj);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  void klp_unpatch_object(struct klp_object *obj)
> >>  {
> >>  	struct klp_func *func;
> >> +	struct klp_hook *hook;
> >> +	int ret;
> >>  
> >>  	klp_for_each_func(obj, func)
> >>  		if (func->patched)
> >>  			klp_unpatch_func(func);
> >>  
> >>  	obj->patched = false;
> >>
> >> +
> >> +	klp_for_each_unpatch_hook(obj, hook) {
> >> +		ret = klp_run_hook(hook, obj);
> >> +		if (ret) {
> >> +			pr_warn("unpatch hook '%p' failed for object '%s'\n",
> >> +				hook, klp_is_module(obj) ? obj->name : "vmlinux");
> >> +		}
> >> +	}
> >> +
> > 
> > It probably does not matter but I would move
> > 
> > 	obj->patched = false;
> > 
> > here. Otherwise, the hooks will see "false" in both cases. In each,
> > case it looks more symetric.
> > 
> > Or is there any reason behind the given order?
> 
> klp_unpatch_func() was just executed for all klp_object functions, so
> IMHO a value of obj->patched = false reflects reality.  Is this not
> symmetric:
> 
> klp_patch_object()
>   klp_for_each_patch_hook     |
>     klp_run_hook              | A - hooks
> 
>   klp_for_each_func           |
>     klp_patch_func            |
>   obj->patched = true         | B - patching
> 
> klp_unpatch_object()
>   klp_for_each_func           |
>     klp_unpatch_func          |
>   obj->patched = false        | B' - unpatching
> 
>   klp_for_each_unpatch_hook   |
>     klp_run_hook              | A' - hooks
> 
> I didn't really have a strong preference here... only for what would be
> most intuitive.

My feeling is that running the hooks is part of the patch
enabling/disabling process. They do thinks that cannot be
done by "simply" registering/unregistering the ftrace handlers.
Therefore the obj->patched state should reflect the state
of the hooks as well.

But again. This is a minor problem. The order is not really
important because everything is synchronized using the klp_mutex.

Best Regards,
Petr