From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751436AbcBEEME (ORCPT ); Thu, 4 Feb 2016 23:12:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59881 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750989AbcBEEMB (ORCPT ); Thu, 4 Feb 2016 23:12:01 -0500 Date: Thu, 4 Feb 2016 23:11:57 -0500 From: Jessica Yu To: Petr Mladek Cc: Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Rusty Russell , Steven Rostedt , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: livepatch/module: remove livepatch module notifier Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160204143934.GZ3305@pathway.suse.cz> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ 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? 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? Thanks, Jessica