From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933008AbcA2UE4 (ORCPT ); Fri, 29 Jan 2016 15:04:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33881 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932960AbcA2UEz (ORCPT ); Fri, 29 Jan 2016 15:04:55 -0500 Date: Fri, 29 Jan 2016 15:04:51 -0500 From: Jessica Yu To: Josh Poimboeuf Cc: Miroslav Benes , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Steven Rostedt , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: livepatch: Implement separate coming and going module notifiers Message-ID: <20160129200451.GB14026@packer-debian-8-amd64.digitalocean.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160129173010.GA19101@treble.redhat.com> 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 +++ Josh Poimboeuf [29/01/16 11:30 -0600]: >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; >+ } If we go this route, should we should print a big warning ("Livepatch couldn't patch loading module X") instead of aborting the module load completely? >+ > 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; > >-- >Josh