From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756691AbcA2R6j (ORCPT ); Fri, 29 Jan 2016 12:58:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34478 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932863AbcA2R6h (ORCPT ); Fri, 29 Jan 2016 12:58:37 -0500 Date: Fri, 29 Jan 2016 11:58:34 -0600 From: Josh Poimboeuf To: Steven Rostedt Cc: Miroslav Benes , Jessica Yu , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Rusty Russell Subject: Re: [PATCH 1/2] livepatch: Implement separate coming and going module notifiers Message-ID: <20160129175834.GB19101@treble.redhat.com> References: <1454049827-3726-1-git-send-email-jeyu@redhat.com> <1454049827-3726-2-git-send-email-jeyu@redhat.com> <20160129173010.GA19101@treble.redhat.com> <20160129124014.204020ed@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160129124014.204020ed@gandalf.local.home> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote: > [ Added Rusty, as he's still maintainer of the module code ] > > On Fri, 29 Jan 2016 11:30:10 -0600 > Josh Poimboeuf wrote: > > > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote: > > > Otherwise than that it looks good. I agree there are advantages to split > > > the notifiers. For example we can replace the coming one with the function > > > call somewhere in load_module() to improve error handling if the patching > > > fails while loading a module. This would be handy with a consistency model > > > in the future. > > > > Yeah, we'll need something like that eventually. Though we'll need to > > make sure that ftrace_module_enable() is still called beforehand, after > > setting MODULE_STATE_COMING state, due to the race described in 5156dca. > > > > Something like: > > > > [note: klp_module_notify_coming() is replaced with klp_module_enable()] > > > > diff --git a/kernel/module.c b/kernel/module.c > > index 8358f46..aeabd81 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info) > > mod->state = MODULE_STATE_COMING; > > mutex_unlock(&module_mutex); > > > > + ftrace_module_enable(mod); > > + err = klp_module_enable(mod); > > + if (err) { > > + ftrace_release_mod(mod); > > + return err; > > + } > > + > > blocking_notifier_call_chain(&module_notify_list, > > MODULE_STATE_COMING, mod); > > return 0; > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index eca592f..c42cf37 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self, > > struct module *mod = data; > > > > switch (val) { > > - case MODULE_STATE_COMING: > > - ftrace_module_enable(mod); > > - break; > > case MODULE_STATE_GOING: > > ftrace_release_mod(mod); > > break; > > If we end up doing something like this, I would just say punt and have > the ftrace code be hardcoded into the module code and remove the > notifiers completely. ftrace (and live kernel patching for that matter) > are rather special. They are not a filesystem or driver. They are core > utilities and having them called directly from the module code may be > prudent and better to understand and control. Agreed, and we might as well make this change now to avoid more churn later. > > Note, you still need to have prototypes for ftrace_module_enable() and > a stub when ftrace is not configured. Same goes for klp_module_enable(). > > -- Steve > -- Josh