Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Julien Thierry <jthierry@redhat.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Miroslav Benes <mbenes@suse.cz>,
	Joe Lawrence <joe.lawrence@redhat.com>,
	Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>,
	Nicolai Stange <nstange@suse.de>,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [POC 02/23] livepatch: Split livepatch modules per livepatched object
Date: Tue, 28 Jan 2020 13:16:53 +0100
Message-ID: <20200128121653.72mhdqnfwtw7kifr@pathway.suse.cz> (raw)
In-Reply-To: <af90531e-219c-3515-1dc8-d86191902ea4@redhat.com>

On Tue 2020-01-21 11:11:45, Julien Thierry wrote:
> Hi Petr,
> 
> On 1/17/20 3:03 PM, Petr Mladek wrote:
> > One livepatch module allows to fix vmlinux and any number of modules
> > while providing some guarantees defined by the consistency model.
> > 
> > The solution is to split the livepatch module per livepatched
> > object (vmlinux or module). Then both livepatch module and
> > the livepatched modules could get loaded and removed at the
> > same time.
> > 
> > The livepatches for modules are put into separate source files
> > that define only struct klp_object() and call the new klp_add_object()
> > in the init() callback. The name of the module follows the pattern:
> > 
> >    <patch_name>__<object_name>
> > 
> 
> Is that a requirement? Or is it just the convention followed for the current
> tests?

This naming pattern is enforced by the code. The reason is to
distinguish the purpose of each livepatch module.

   + Livepatch module for "vmlinux" and the related livepatch modules
     for other objects.

   + Different livepatches (versions) that might be installed at the
     same time. This happens even with cumulative livepatches.


It is important for the functionality:

   + Consistency checks that all and right livepatch modules are
     loaded.

   + Automatic loading of livepatch modules for modules when the patched
     module is being loaded.

But it should be "clear" even for humans because the livepatch modules are
listed by lsmod, ...

Of course, we could talk about other naming scheme, another approach.


> > @@ -844,21 +822,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
> >   	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> >   	init_completion(&patch->finish);
> > -	klp_for_each_object_static(patch, obj) {
> 
> I think we can get rid of klp_for_each_object_static(), no? Now the
> klp_patch is only associated to a single klp_object, so everything will be
> dynamic. Is this correct?

Yes, the macro klp_for_each_object_static() is not longer needed.

Just to be sure. It would be better to say that all klp_object
structures will be in the linked lists only.

Most structures are still defined statically. The name "dynamic" is
used for the dynamically allocated structures. They are used for
"nop" functions that might be needed when doing atomic replace
of cumulative patches and functions that are not longer patched.
See obj->dynamic and func->nop.


> > @@ -991,12 +958,12 @@ int klp_enable_patch(struct klp_patch *patch)
> >   {
> >   	int ret;
> > -	if (!patch || !patch->mod)
> > +	if (!patch || !patch->obj || !patch->obj->mod)
> >   		return -EINVAL;
> > -	if (!is_livepatch_module(patch->mod)) {
> > +	if (!is_livepatch_module(patch->obj->mod)) {
> >   		pr_err("module %s is not marked as a livepatch module\n",
> > -		       patch->mod->name);
> > +		       patch->obj->patch_name);
> 
> Shouldn't that be "patch->obj->mod->name" ?

They are actually the same. Note that it is redundant only in
struct klp_object that is in the livepatch module for vmlinux.

Hmm, it might be possible to get rid of it after I added the array
patch->obj_names. But I would prefer to keep it as a consistency
check.

One big drawback of the split modules approach is that there are
suddenly many more livepatch modules. The kernel code has to make
sure always the right ones are loaded. It is great to have some
cross-checks.


> >   		return -EINVAL;
> >   	}

> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index f6310f848f34..78e3280560cd 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -147,7 +145,7 @@ void klp_cancel_transition(void)
> >   		return;
> >   	pr_debug("'%s': canceling patching transition, going to unpatch\n",
> > -		 klp_transition_patch->mod->name);
> > +		 klp_transition_patch->obj->patch_name);
> >   	klp_target_state = KLP_UNPATCHED;
> >   	klp_complete_transition();
> > @@ -468,7 +466,7 @@ void klp_start_transition(void)
> >   	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> >   	pr_notice("'%s': starting %s transition\n",
> > -		  klp_transition_patch->mod->name,
> > +		  klp_transition_patch->obj->patch_name,
> 
> Isn't the transition per livepatched module rather than per-patch now?
> If so, would it make more sense to display also the name of the module being
> patched/unpatched?

The transition still happens for the entire livepatch defined by
struct klp_patch. All needed livepatch modules for the other objects
are loaded before the transition starts, see the patch 17/24
("livepatch: Load livepatches for modules when loading the main
livepatch").

> >   		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> >   	/*

Best Regards,
Petr

PS: Julien,

first, thanks a lot for looking at the patchset.

I am going to answer questions and comments that are related to
the overall design. The most important question is if the split
livepatch modules are the way to go. I hope that this patchset
shows possible wins and catches so that we could decide if it
is worth the effort.

Anyway, feel free to comment even details when you notice
a mistake. There might be some catches that I missed, ...

Best Regards,
Petr

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
2020-01-21 11:11   ` Julien Thierry
2020-01-17 15:03 ` [POC 02/23] livepatch: Split livepatch modules per livepatched object Petr Mladek
2020-01-21 11:11   ` Julien Thierry
2020-01-28 12:16     ` Petr Mladek [this message]
2020-01-17 15:03 ` [POC 03/23] livepatch: Better checks of struct klp_object definition Petr Mladek
2020-01-21 11:27   ` Julien Thierry
2020-01-17 15:03 ` [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally Petr Mladek
2020-01-17 15:03 ` [POC 05/23] livepatch: Initialize and free livepatch submodule Petr Mladek
2020-01-21 11:58   ` Julien Thierry
2020-01-17 15:03 ` [POC 06/23] livepatch: Enable the " Petr Mladek
2020-01-17 15:03 ` [POC 07/23] livepatch: Remove obsolete functionality from klp_module_coming() Petr Mladek
2020-01-17 15:03 ` [POC 08/23] livepatch: Automatically load livepatch module when the patch module is loaded Petr Mladek
2020-01-17 15:03 ` [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load Petr Mladek
2020-01-22 18:51   ` Julien Thierry
2020-01-17 15:03 ` [POC 10/23] livepatch: Handle modprobe exit code Petr Mladek
2020-01-17 15:03 ` [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules Petr Mladek
2020-01-22 10:15   ` Julien Thierry
2020-01-17 15:03 ` [POC 12/23] livepatch: Automatically remove livepatch module when the object is freed Petr Mladek
2020-01-17 15:03 ` [POC 13/23] livepatch: Remove livepatch module when the livepatched module is unloaded Petr Mladek
2020-01-17 15:03 ` [POC 14/23] livepatch: Never block livepatch modules when the related module is being removed Petr Mladek
2020-01-17 15:03 ` [POC 15/23] livepatch: Prevent infinite loop when loading livepatch module Petr Mladek
2020-01-17 15:03 ` [POC 16/23] livepatch: Add patch into the global list early Petr Mladek
2020-01-17 15:03 ` [POC 17/23] livepatch: Load livepatches for modules when loading the main livepatch Petr Mladek
2020-01-17 15:03 ` [POC 18/23] module: Refactor add_unformed_module() Petr Mladek
2020-01-17 15:03 ` [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux" Petr Mladek
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
2020-01-18 10:29   ` kbuild test robot
2020-01-17 15:03 ` [POC 21/23] livepatch: Remove obsolete arch_klp_init_object_loaded() Petr Mladek
2020-01-17 15:03 ` [POC 22/23] livepatch/module: Remove obsolete copy_module_elf() Petr Mladek
2020-01-17 15:03 ` [POC 23/23] module: Remove obsolete module_disable_ro() Petr Mladek

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200128121653.72mhdqnfwtw7kifr@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@redhat.com \
    --cc=jthierry@redhat.com \
    --cc=kamalesh@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=nstange@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git