From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754280AbdGNNXd (ORCPT ); Fri, 14 Jul 2017 09:23:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53982 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753942AbdGNNXb (ORCPT ); Fri, 14 Jul 2017 09:23:31 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A61927DCF0 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=joe.lawrence@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A61927DCF0 Date: Fri, 14 Jul 2017 09:23:29 -0400 From: Joe Lawrence To: Josh Poimboeuf Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina , Miroslav Benes , Petr Mladek , Chris J Arges Subject: Re: [PATCH] livepatch: add (un)patch hooks Message-ID: <20170714132329.54f763zbc7erlm5h@redhat.com> References: <1499868600-10176-1-git-send-email-joe.lawrence@redhat.com> <1499868600-10176-2-git-send-email-joe.lawrence@redhat.com> <20170714014640.nhoowbrleu6kdka2@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170714014640.nhoowbrleu6kdka2@treble> User-Agent: Mutt/1.6.2-neo (2016-08-08) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 14 Jul 2017 13:23:31 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 13, 2017 at 08:46:40PM -0500, Josh Poimboeuf wrote: > Date: Thu, 13 Jul 2017 20:46:40 -0500 > From: Josh Poimboeuf > To: Joe Lawrence > Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu > , Jiri Kosina , Miroslav Benes > , Petr Mladek , Chris J Arges > > Subject: Re: [PATCH] livepatch: add (un)patch hooks > User-Agent: Mutt/1.6.0.1 (2016-04-01) > > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote: > > When the livepatch core executes klp_(un)patch_object, call out to a > > livepatch-module specified array of callback hooks. These hooks provide > > a notification mechanism for livepatch modules when klp_objects are > > (un)patching. This may be most interesting when another kernel module > > is a klp_object target and the livepatch module needs to execute code > > after the target is loaded, but before its module_init code is run. > > And it's also useful for vmlinux. Patch module load/unload is separate > from enable/disable, so the module init/exit functions can't be used for > patch-specific changes (e.g., global data changes). > > > The patch-hook executes right before patching objects and the > > unpatch-hook executes right after unpatching objects. > > > > Signed-off-by: Joe Lawrence > > Thanks for posting it. We found this to be a useful feature in the > past, not quite as useful as shadow data, but still good to have for > certain cases. > > Instead of "load hooks" I think it would be more accurate to call them > "enable/disable hooks". (Maybe "callbacks" would be better than > "hooks"? Not sure...) Hi Josh, I hesitataed in calling them "enable/disable" hooks as I associated those terms at the patch level -- a livepatch might be enabled, but callbacks for a module may not occur until its actually loaded. (I'm fine with whatever is most intuitive to the livepatching collective :) "Callbacks" vs. "hooks" is a good point though, as the latter has negative connotations, especially when callers of this facility will be mostly out of tree. > Even better, we might want to be specific: "pre enable hooks" and "post > disable hooks". (Or "pre patch hooks" and "post unpatch hooks"?) > Because we might eventually decide we need the corresponding "post > enable hooks" and "pre disable hooks" as well. "Pre-patch" and "post-unpatch" are a bit wordy, but a good description. I already felt it was important enough to document the order of operations in the doc file and commit msg, so I like this idea. > For the enable case, I think it would be a nice feature if we checked > the return code and aborted the patching operation on error. I think > that should be easy enough. Yeah, that should be easy. To be specific, you're only talking about the patching operation on the associated klp_object, not the entire klp_patch right? > For the unload case, it's too late to do anything, so I'd say a void > return code would be better. Otherwise it implies that we actually do > something about it. Maybe in that case we can leave it up to the user > to decide whether to print an error or WARN() or whatever. Good point. I can change that in v2. Thanks, -- Joe