livepatch: clean up klp_find_object_module() usage: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core.  It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.

[...]

> > +/* sets obj->mod if object is not vmlinux and module is found */
> > +static bool klp_find_object_module(struct klp_object *obj)
> > +{
> > +	if (!obj->name)
> > +		return 1;
> > +
> > +	mutex_lock(&module_mutex);
> > +	/*
> > +	 * We don't need to take a reference on the module here because we have
> > +	 * the klp_mutex, which is also taken by the module notifier.  This
> > +	 * prevents any module from unloading until we release the klp_mutex.
> > +	 */
> > +	obj->mod = find_module(obj->name);
> > +	mutex_unlock(&module_mutex);
> > +
> > +	return !!obj->mod;
>
> I know that this is effective to return boolean here because
> it handles also patch against the kernel core (vmlinux). But
> the logic looks tricky. I would prefer a cleaner design and
> use this function only to set obj->mod.
>
> I wanted to see how it would look like, so I will send a patch for
> this in a separate mail.

The patch is below. Of course, merge it into the original
patch if you agree with the idea, please.

From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.cz>
Date: Fri, 28 Nov 2014 15:32:27 +0100
Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage

The function klp_find_object_module() looks quite tricky. It has two effects:
sets obj->mod and decides whether the module is available or not. The second
effect is the tricky part because it handles also the code kernel code "vmlinux"
and is not module related. It causes returning bool, and doing the crazy double
negation.

This patch tries to make a bit cleaner design:

1. klp_find_object_module() handles only obj->mod. It returns
   the pointer or NULL.

2. It modifies klp_enable_object() to do nothing when the related
   module has not been loaded yet.

3. The result is that the return value klp_find_object_module() is
   not used in the end.

We lose a check for potential klp_enable_object() misuse but it makes the code
easier. In fact, the check for unloaded module is rather long. We might want
to make it easier using some extra flag or another state of the object.
Such flag might be used for the check of misuse.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
livepatch: do relocation when initializing the patch: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core.  It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.

[...]

> > +static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> > +{
> > +	struct klp_func *func;
> > +	int ret;
> > +
> 	if (WARN_ON(obj->state != KLP_DISABLED))
> 		return -EINVAL;
>
> > +	if (WARN_ON(!obj->mod && obj->name))
> > +		return -EINVAL;
> > +
> > +	if (obj->relocs) {
> > +		ret = klp_write_object_relocations(pmod, obj);
>
> I was curious why the relocation is here. I think that the motivation
> was to safe the call when handling comming modules.
>
> IMHO, more clean solution would be to do this in klp_init_object().
> It will also cause removing the "pmod" parameter and this function
> will be more symmetric with klp_disable_object();
>
> I was curious how it would work, so I prepared a patch. I will send
> it in separate mail.

The patch is below. Again, merge it into the original patch if you
agree with the idea, please.

From ba3b08938b6f6f1a041af645ff43825f2ec1222f Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.cz>
Date: Fri, 28 Nov 2014 15:57:23 +0100
Subject: [PATCH 2/2] livepatch: do relocation when initializing the patch

I think that the relocation is done in klp_enable_object() because
it safes the duplication in klp_module_notify_coming().

But I think that the relocation logically belongs to the init phase.
It will remove duplicate relocation if we allow repeated enable/disable
calls for the same patch. Also it removes the extra "pmod" parameter
from klp_enable_object() and makes it more symmetric with klp_disable_object().

This patch moves also the klp_find_object_module() to the init phase
where it belongs.

Finally, it moves some checks from callers to klp_write_object_relocation().
They must be there in each case. It makes the code easier when calling
from more locations.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
Miroslav's suggested changes to Petr's patch
add comments to klp_write_module_reloc()
explicitly include asm/elf.h for reloc types
make readonly bool and optimize
fixup kerel-doc comment for struct klp_object
rename struct klp_reloc fields
add WARN_ON checks
generate compile error when live patching is disabled
let __klp_disable_patch() handle disable-when-disabled
mutex protect all of klp_init_patch(), avoid race with module notifier
add notrace to ftrace handler to avoid recursion
fixup else braces
remove redundant obj->mod assignment (again)
handle NULL obj->name in kobject_create_and_add()
remove redundant patch->objs == NULL check
add asm/livepatch.h include
add comment for numpages
add missing colons to structure kernel-doc