linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Jessica Yu <jeyu@redhat.com>
Cc: Miroslav Benes <mbenes@suse.cz>,
	jpoimboe@redhat.com, jikos@kernel.org, jslaby@suse.cz,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	huawei.libin@huawei.com, minfei.huang@yahoo.com
Subject: Re: livepatch: allow removal of a disabled patch
Date: Wed, 8 Jun 2016 10:10:18 +0200	[thread overview]
Message-ID: <20160608081018.GF10857@pathway.suse.cz> (raw)
In-Reply-To: <20160607223950.GA27442@packer-debian-8-amd64.digitalocean.com>

On Tue 2016-06-07 18:39:51, Jessica Yu wrote:
> +++ Petr Mladek [07/06/16 11:36 +0200]:
> >On Wed 2016-06-01 10:31:59, Miroslav Benes wrote:
> >>Currently we do not allow patch module to unload since there is no
> >>method to determine if a task is still running in the patched code.
> >>
> >>The consistency model gives us the way because when the unpatching
> >>finishes we know that all tasks were marked as safe to call an original
> >>function. Thus every new call to the function calls the original code
> >>and at the same time no task can be somewhere in the patched code,
> >>because it had to leave that code to be marked as safe.
> >>
> >>We can safely let the patch module go after that.
> >>
> >>Completion is used for synchronization between module removal and sysfs
> >>infrastructure in a similar way to commit 942e443127e9 ("module: Fix
> >>mod->mkobj.kobj potentially freed too early").
> >>
> >>Note that we still do not allow the removal for immediate model, that is
> >>no consistency model. The module refcount may increase in this case if
> >>somebody disables and enables the patch several times. This should not
> >>cause any harm.
> >>
> >>With this change a call to try_module_get() is moved to
> >>__klp_enable_patch from klp_register_patch to make module reference
> >>counting symmetric (module_put() is in a patch disable path) and to
> >>allow to take a new reference to a disabled module when being enabled.
> >>
> >>Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex
> >>lock protection to prevent a deadlock situation when
> >>klp_unregister_patch is called and sysfs directories are removed. There
> >>is no need to do the same for other kobject_put() callsites as we
> >>currently do not have their sysfs counterparts.
> >>
> >>Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> >>---
> >>Based on Josh's v2 of the consistency model.
> >>
> >>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >>index d55701222b49..a649186fb4af 100644
> >>--- a/kernel/livepatch/core.c
> >>+++ b/kernel/livepatch/core.c
> >>@@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> >>
> >> 	mutex_lock(&klp_mutex);
> >>
> >>+	if (!klp_is_patch_registered(patch)) {
> >>+		/*
> >>+		 * Module with the patch could either disappear meanwhile or is
> >>+		 * not properly initialized yet.
> >>+		 */
> >>+		ret = -EINVAL;
> >>+		goto err;
> >>+	}
> >>+
> >> 	if (patch->enabled == val) {
> >> 		/* already in requested state */
> >> 		ret = -EINVAL;
> >>@@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
> >> 	mutex_lock(&klp_mutex);
> >>
> >> 	patch->enabled = false;
> >>+	init_completion(&patch->finish);
> >>
> >> 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> >> 				   klp_root_kobj, "%s", patch->mod->name);
> >>-	if (ret)
> >>-		goto unlock;
> >>+	if (ret) {
> >>+		mutex_unlock(&klp_mutex);
> >>+		return ret;
> >>+	}
> >>
> >> 	klp_for_each_object(patch, obj) {
> >> 		ret = klp_init_object(patch, obj);
> >>@@ -720,9 +744,12 @@ static int klp_init_patch(struct klp_patch *patch)
> >>
> >> free:
> >> 	klp_free_objects_limited(patch, obj);
> >>-	kobject_put(&patch->kobj);
> >>-unlock:
> >>+
> >> 	mutex_unlock(&klp_mutex);
> >>+
> >
> >Just for record. The sysfs entry exists but patch->list is not
> >initialized in this error path. Therefore we could write into
> >
> >  /sys/.../livepatch_foo/enable
> >
> >and call enabled_store(). It is safe because enabled_store() calls
> >klp_is_patch_registered() and it does not need patch->list for this
> >check. Kudos for the klp_is_patch_registered() implementation.
> >
> >I write this because it is not obvious and it took me some time
> >to verify that we are on the safe side.
> >
> >Well, I would feel more comfortable if we initialize patch->list
> >above.
> 
> Hi Petr,
> 
> I'm a bit unclear on this, can you clarify what you mean by initialize
> patch->list? I don't think any extra initialization is needed here to
> be able use a patch->list node with an existing list (klp_patches),
> since this field is embedded and the klp_patches list header is
> already statically initialized with LIST_HEAD.

I meant to call

	INIT_LIST_HEAD(&patch->list);

around the existing initialization:

	patch->enabled = false;
	init_completion(&patch->finish);


The list is zeroed by default. This is true at least in
the livepatch_sample.c. It means that it is not a valid
list head without an extra initialization.

Also note that we do a check for list_empty(&patch->list) in
klp_free_patch(). This check is useless if we do not initialize
the list.

Best Regards,
Petr

  reply	other threads:[~2016-06-08  8:10 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-01  8:31 [PATCH v2] " Miroslav Benes
2016-06-01 11:29 ` Christopher Arges
2016-06-01 11:43   ` Miroslav Benes
2016-06-07  9:36 ` Petr Mladek
2016-06-07 22:39   ` Jessica Yu
2016-06-08  8:10     ` Petr Mladek [this message]
  -- strict thread matches above, loose matches on Subject: below --
2016-05-04  2:39 [RFC PATCH] " Josh Poimboeuf
2016-05-04  3:36 ` Josh Poimboeuf
2016-05-04 11:58   ` Miroslav Benes
2016-05-04 13:14     ` Josh Poimboeuf
2016-05-04 14:35       ` Miroslav Benes
2016-05-04 16:14         ` Josh Poimboeuf
2016-05-05  8:28           ` Miroslav Benes
2016-05-05 13:27             ` Josh Poimboeuf
2016-05-05 14:25               ` Miroslav Benes
2016-05-05 15:04                 ` Josh Poimboeuf
2016-05-06  0:42                   ` Jessica Yu
2016-05-06  7:51                     ` Miroslav Benes

Reply instructions:

You may reply publicly 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=20160608081018.GF10857@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=huawei.libin@huawei.com \
    --cc=jeyu@redhat.com \
    --cc=jikos@kernel.org \
    --cc=jpoimboe@redhat.com \
    --cc=jslaby@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=minfei.huang@yahoo.com \
    --subject='Re: livepatch: allow removal of a disabled patch' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).