From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752383AbcBEJQv (ORCPT ); Fri, 5 Feb 2016 04:16:51 -0500 Received: from mx2.suse.de ([195.135.220.15]:54061 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752345AbcBEJP7 (ORCPT ); Fri, 5 Feb 2016 04:15:59 -0500 Date: Fri, 5 Feb 2016 10:15:56 +0100 (CET) From: Miroslav Benes To: Jessica Yu cc: Petr Mladek , Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Rusty Russell , 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> Message-ID: 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: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 4 Feb 2016, Jessica Yu wrote: > +++ 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. > > @@ -3649,16 +3659,16 @@ static int load_module(struct load_info *info, const > char __user *uargs, > > + coming_cleanup: > + blocking_notifier_call_chain(&module_notify_list, > + MODULE_STATE_GOING, mod); > + klp_module_disable(mod); > bug_cleanup: > /* module_bug_cleanup needs module_mutex protection */ > mutex_lock(&module_mutex); > module_bug_cleanup(mod); > mutex_unlock(&module_mutex); > > - blocking_notifier_call_chain(&module_notify_list, > - MODULE_STATE_GOING, mod); > - klp_module_disable(mod); > - > /* we can't deallocate the module until we clear memory protection */ > module_disable_ro(mod); > module_disable_nx(mod); > > Does all this look ok? Hm, there is an another option. We can cover the needed error handling in complete_formation(). So for the first error there (verify_export_symbols() fails) we need to only release module_mutex. For our second error we need more. We would call module_bug_cleanup() under module_mutex, module_disable_{ro,nx} and going notifiers. Is this correct? It would be hidden in complete_formation() this way and cleaner in my opinion. There is some code duplication though. > 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? This could break something or introduce a race somewhere. Git grep says there are several checks for MODULE_STATE_GOING through out the kernel which need to be checked. Regards, Miroslav