From: Miroslav Benes <mbenes@suse.cz>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: jeyu@redhat.com, jikos@kernel.org, pmladek@suse.com,
jslaby@suse.cz, live-patching@vger.kernel.org,
linux-kernel@vger.kernel.org, huawei.libin@huawei.com,
minfei.huang@yahoo.com
Subject: Re: [RFC PATCH] livepatch: allow removal of a disabled patch
Date: Tue, 3 May 2016 10:16:01 +0200 (CEST) [thread overview]
Message-ID: <alpine.LNX.2.00.1605031008550.23480@pobox.suse.cz> (raw)
In-Reply-To: <20160502150830.nld53ffxrzeqbojt@treble>
On Mon, 2 May 2016, Josh Poimboeuf wrote:
> On Mon, May 02, 2016 at 01:57:22PM +0200, 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 patching
> > finishes we know that all tasks were marked as safe to call a new
> > patched function. Thus every new call to the function calls the new
> > patched code and at the same time no task can be somewhere in the old
> > code, because it had to leave that code to be marked as safe.
> >
> > We can safely let the patch module go after that.
>
> I found this a little confusing because it talks about patching, whereas
> we really want to remove the patch module after _unpatching_ it.
You're right. I'll rephrase that.
> > Completion is used for synchronization between module removal and sysfs
> > infrastructure. Note that we still do not allow the removal for
> > immediate model, that is no consistency model.
> >
> > 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.
>
> One minor issue here: for patch->immediate, if somebody enabled and
> disabled the patch several times, the module ref count would keep
> increasing. Probably not a big deal... maybe we don't need to worry
> about it.
That is unfortunately true. I don't have a solution in my pocket which
would be 100% reliable. At the same time I don't see a practical problem.
Yes, refcount could overflow, but that shouldn't be a problem, should it?
Anyway, I'll note it in the changelog.
> > 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.
> >
> > There are still two things which need to be discussed.
> >
> > 1. Do we really need a completion? If I am not missing something
> > kobject_del() always waits for sysfs callers to leave thanks to kernfs
> > active protection. The only problem is in kobject_release() when
> > CONFIG_DEBUG_KOBJECT_RELEASE=y. In this case kobject_delayed_cleanup()
> > is delay-scheduled, which is a problem for us. For this we definitely
> > need the completion, right? JiriS, is this correct?
>
> Sysfs and kobjects hurt my brain; I need to take a refresher course, so
> I don't have an answer to this yet :-) Maybe Jiri S can enlighten us.
:)
> > 2. I refuse to remove a module when patch->immediate is set or one of
> > its func->immediate is set. We can do slightly better, because we can
> > allow the module to go if patch->immediate is false and all func with
> > immediate set to true are from some object which is not loaded. This is
> > for discussion because it needs more changes in the code (I'd like to
> > call klp_is_object_loaded() which is somewhere else and so on).
>
> I don't think we need to worry about doing that unless somebody
> complains about it. It's kind of obscure and patch removal isn't very
> important anyway...
Agreed.
Thanks,
Miroslav
next prev parent reply other threads:[~2016-05-03 8:16 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-02 11:57 [RFC PATCH] livepatch: allow removal of a disabled patch Miroslav Benes
2016-05-02 15:08 ` Josh Poimboeuf
2016-05-03 8:16 ` Miroslav Benes [this message]
2016-05-31 23:13 ` Jiri Kosina
2016-05-03 21:37 ` Josh Poimboeuf
2016-05-03 22:31 ` Jiri Kosina
2016-05-04 2:39 ` 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-05 21:08 ` Jiri Kosina
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=alpine.LNX.2.00.1605031008550.23480@pobox.suse.cz \
--to=mbenes@suse.cz \
--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=minfei.huang@yahoo.com \
--cc=pmladek@suse.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).