From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756867AbcA2UU0 (ORCPT ); Fri, 29 Jan 2016 15:20:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40937 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752766AbcA2UUZ (ORCPT ); Fri, 29 Jan 2016 15:20:25 -0500 Date: Fri, 29 Jan 2016 14:20:23 -0600 From: Josh Poimboeuf To: Jessica Yu 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: <20160129202023.GF19101@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> <20160129200451.GB14026@packer-debian-8-amd64.digitalocean.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160129200451.GB14026@packer-debian-8-amd64.digitalocean.com> 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 03:04:51PM -0500, Jessica Yu wrote: > +++ 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? I think aborting the module load is better. Otherwise the patch would be applied in an inconsistent state. Which might not be all that bad now, since we don't have a consistency model anyway; but it could be disastrous once we get one, if somebody is relying on that consistency. -- Josh