From: Miroslav Benes <mbenes@suse.cz>
To: Jessica Yu <jeyu@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
Seth Jennings <sjenning@redhat.com>,
Jiri Kosina <jikos@kernel.org>, Vojtech Pavlik <vojtech@suse.com>,
Rusty Russell <rusty@rustcorp.com.au>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier
Date: Thu, 4 Feb 2016 18:29:46 +0100 (CET) [thread overview]
Message-ID: <alpine.LNX.2.00.1602041822070.10689@pobox.suse.cz> (raw)
In-Reply-To: <1454375856-27757-3-git-send-email-jeyu@redhat.com>
On Mon, 1 Feb 2016, Jessica Yu wrote:
> +/* Called from the module loader during module coming/going states */
> +extern int klp_module_enable(struct module *mod);
> +extern void klp_module_disable(struct module *mod);
We do not use 'extern' keyword in header files. It is redundant.
Unfortunately, the situation differs among header files and it is hard to
be consistent.
> + /*
> + * Each module has to know that the coming handler has
> + * been called. We never know what module will get
> + * patched by a new patch.
> + */
> + mod->klp_alive = true;
This comment should fixed too.
Note: we still need klp_alive, because the race is still there even
without notifiers.
> +void klp_module_disable(struct module *mod)
> {
> - int ret;
> - struct module *mod = data;
> struct klp_patch *patch;
> struct klp_object *obj;
>
> - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> - return 0;
> + if (mod->state != MODULE_STATE_GOING)
> + return;
This is similar to what Petr proposed. We must be in MODULE_STATE_GOING
here. We could WARN here and return.
> diff --git a/kernel/module.c b/kernel/module.c
> index b05d466..71c77ed 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
> mutex_unlock(&module_mutex);
>
> ftrace_module_enable(mod);
> + err = klp_module_enable(mod);
> + if (err)
> + goto out;
> +
if (err)
return err;
module_mutex is already unlocked so no need to jump to out.
Apart from these minor things and Petr's remarks it looks ok.
Thanks for fixing this.
Miroslav
next prev parent reply other threads:[~2016-02-04 17:30 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-02 1:17 [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jessica Yu
2016-02-02 1:17 ` [PATCH v2 1/2] ftrace/module: remove ftrace module notifier Jessica Yu
2016-02-04 13:27 ` Petr Mladek
2016-02-04 14:18 ` Steven Rostedt
2016-02-04 15:21 ` Petr Mladek
2016-02-04 15:33 ` Steven Rostedt
2016-02-02 1:17 ` [PATCH v2 2/2] livepatch/module: remove livepatch " Jessica Yu
2016-02-04 14:39 ` Petr Mladek
2016-02-04 14:56 ` Steven Rostedt
2016-02-04 16:47 ` Miroslav Benes
2016-02-05 4:11 ` Jessica Yu
2016-02-05 9:15 ` Miroslav Benes
2016-02-05 10:06 ` Petr Mladek
2016-02-08 0:34 ` Rusty Russell
2016-02-04 17:29 ` Miroslav Benes [this message]
2016-02-04 20:55 ` [PATCH v2 2/2] " Josh Poimboeuf
2016-02-05 8:59 ` Miroslav Benes
2016-02-04 10:43 ` [PATCH v2 0/2] Fix ordering of ftrace/livepatch calls on module load and unload Jiri Kosina
2016-02-04 13:29 ` Steven Rostedt
2016-02-05 1:17 ` Rusty Russell
-- strict thread matches above, loose matches on Subject: below --
2016-03-11 20:03 [PATCH v2 0/2] Livepatch module notifier cleanup Jessica Yu
2016-03-11 20:03 ` [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Jessica Yu
2016-03-14 15:06 ` Petr Mladek
2016-03-14 20:01 ` Josh Poimboeuf
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.1602041822070.10689@pobox.suse.cz \
--to=mbenes@suse.cz \
--cc=jeyu@redhat.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=rostedt@goodmis.org \
--cc=rusty@rustcorp.com.au \
--cc=sjenning@redhat.com \
--cc=vojtech@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 read-only IMAP folder(s) and NNTP newsgroup(s).