From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754409AbcBHBwK (ORCPT ); Sun, 7 Feb 2016 20:52:10 -0500 Received: from ozlabs.org ([103.22.144.67]:45527 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754199AbcBHBwI (ORCPT ); Sun, 7 Feb 2016 20:52:08 -0500 From: Rusty Russell To: Jessica Yu , Petr Mladek Cc: Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Steven Rostedt , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: livepatch/module: remove livepatch module notifier In-Reply-To: <20160205041157.GB24068@packer-debian-8-amd64.digitalocean.com> References: <1454375856-27757-1-git-send-email-jeyu@redhat.com> <1454375856-27757-3-git-send-email-jeyu@redhat.com> <20160204143934.GZ3305@pathway.suse.cz> <20160205041157.GB24068@packer-debian-8-amd64.digitalocean.com> User-Agent: Notmuch/0.20.2 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 08 Feb 2016 11:04:52 +1030 Message-ID: <87zivcm443.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Jessica Yu writes: > +++ Petr Mladek [04/02/16 15:39 +0100]: >>On Mon 2016-02-01 20:17:36, Jessica Yu wrote: > [ snipped since email is getting long ] >>> diff --git a/kernel/module.c b/kernel/module.c >>> index b05d466..71c77ed 100644 >>> --- a/kernel/module.c >>> +++ b/kernel/module.c >>> @@ -53,6 +53,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, >>> mod->exit(); >>> blocking_notifier_call_chain(&module_notify_list, >>> MODULE_STATE_GOING, mod); >>> + klp_module_disable(mod); >>> ftrace_release_mod(mod); >>> >>> async_synchronize_full(); >>> @@ -3297,6 +3299,7 @@ fail: >>> module_put(mod); >>> blocking_notifier_call_chain(&module_notify_list, >>> MODULE_STATE_GOING, mod); >>> + klp_module_disable(mod); >>> ftrace_release_mod(mod); >>> free_module(mod); >>> wake_up_all(&module_wq); >>> @@ -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 you go out here, you need to revert some some operations >>that are normally done in the bug_cleanup: goto target >>in load_module(). In particular, you need to do: >> >> /* module_bug_cleanup needs module_mutex protection */ >> mutex_lock(&module_mutex); >> module_bug_cleanup(mod); >> mutex_unlock(&module_mutex); >> >> ftrace_release_mod(mod); >> >> /* we can't deallocate the module until we clear memory protection */ >> module_disable_ro(mod); >> module_disable_nx(mod); >> >> >>IMHO, it would make sense to somehow split the complete_formation() function >>and avoid a code duplication in the error paths. > > Argh, thank you for catching that. I think we could split up complete_formation() > into two functions in order to make the error handling work. > > We could probably take out the coming notifier calls, ftrace_module_enable(), > and klp_module_enable() out of complete_formation(), and put them in another > function, maybe called prepare_coming_module(), that would be called right > after complete_formation(). It might look something like this: > > @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs, > err = complete_formation(mod, info); > if (err) > goto ddebug_cleanup; > + err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers > + if (err) // means that klp_module_enable failed > + goto bug_cleanup; > > /* Module is ready to execute: parsing args may do that. */ > after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, > @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs, > unknown_module_param_cb); > if (IS_ERR(after_dashes)) { > err = PTR_ERR(after_dashes); > - goto bug_cleanup; > + goto coming_cleanup; > } else if (after_dashes) { > pr_warn("%s: parameters '%s' after `--' ignored\n", > mod->name, after_dashes); > > Now for the error conditions. If complete_formation() fails, goto > ddebug_cleanup. If prepare_coming_module() fails (at that point, > module_enable_{ro,nx} and module_bug_finalize() have already finished), goto > bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable, > ftrace_module_enable, and the coming notifiers have finished) goto > coming_cleanup. ftrace_release_mod() gets called in the goto free_module label > so we don't have to call it in coming_module. Sounds good. > Also, one last thing, I noticed that module->state isn't > set to MODULE_STATE_GOING anywhere before the going notifier chain is called in > the bug_cleanup label (I think it is still COMING at that point), so the > klp_module_disable call right afterwards would have bailed out because of that. > To be consistent, shouldn't it be set before the going notifiers are called? Good spotting. You could argue that there's a difference between the notifier argument (what we're doing) and the module state (how far it got), but we do set 'mod->state = MODULE_STATE_GOING;' in the initcall fail case, so this should be the same. Patch welcome :) Thanks, Rusty.