From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932605AbbKCUmY (ORCPT ); Tue, 3 Nov 2015 15:42:24 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:59303 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755375AbbKCUmW (ORCPT ); Tue, 3 Nov 2015 15:42:22 -0500 Subject: Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory To: Josh Poimboeuf , Miroslav Benes References: <20151102203241.GF27488@treble.redhat.com> <1446505187-28970-1-git-send-email-chris.j.arges@canonical.com> <20151103145843.GH27488@treble.redhat.com> <20151103165058.GM27488@treble.redhat.com> Cc: live-patching@vger.kernel.org, jeyu@redhat.com, Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-api@vger.kernel.org, linux-kernel@vger.kernel.org From: Chris J Arges X-Enigmail-Draft-Status: N1110 Message-ID: <56391C27.8040104@canonical.com> Date: Tue, 3 Nov 2015 14:42:15 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151103165058.GM27488@treble.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/03/2015 10:50 AM, Josh Poimboeuf wrote: > On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote: >> On Tue, 3 Nov 2015, Josh Poimboeuf wrote: >> >>> On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote: >>>> On Mon, 2 Nov 2015, Chris J Arges wrote: >>>> >>>> [...] >>>> >>>>> +static int klp_get_func_pos_callback(void *data, const char >>>>> *name, + struct module *mod, unsigned long addr) +{ >>>>> + struct klp_find_arg *args = data; + + if ((mod && >>>>> !args->objname) || (!mod && args->objname)) + return 0; + + >>>>> if (strcmp(args->name, name)) + return 0; + + if >>>>> (args->objname && strcmp(args->objname, mod->name)) + return >>>>> 0; + + /* on address match, return 1 to break >>>>> kallsyms_on_each_symbol loop */ + if (args->addr == addr) + >>>>> return 1; + + /* if we don't match addr, count instance of >>>>> named symbol */ + args->count++; + + return 0; +} + +static >>>>> int klp_get_func_pos(struct klp_object *obj, struct klp_func >>>>> *func) +{ + struct klp_find_arg args = { + .objname = >>>>> obj->name, + .name = func->old_name, + .addr = >>>>> func->old_addr, + .count = 0, + }; + + >>>>> mutex_lock(&module_mutex); + >>>>> kallsyms_on_each_symbol(klp_get_func_pos_callback, &args); + >>>>> mutex_unlock(&module_mutex); + + return args.count; +} + >>>>> static int klp_init_func(struct klp_object *obj, struct >>>>> klp_func *func) { INIT_LIST_HEAD(&func->stack_node); >>>>> func->state = KLP_DISABLED; >>>>> >>>>> return kobject_init_and_add(&func->kobj, &klp_ktype_func, - >>>>> &obj->kobj, "%s", func->old_name); + &obj->kobj, >>>>> "%s,%d", func->old_name, + klp_get_func_pos(obj, >>>>> func)); } >>>> >>>> There is a problem which I missed before. klp_init_func() is >>>> called before klp_find_verify_func_addr() in klp_init_object(). >>>> This means that func->old_addr is either not verified yet or >>>> worse it is still 0. This means that >>>> klp_get_func_pos_callback() never returns 1 and is thus called >>>> on each symbol. So if you for example patched >>>> cmdline_proc_show the resulting directory in sysfs would be >>>> called cmdline_proc_show,1 because addr is never matched. Had >>>> old_addr been specified the name would have been probably >>>> correct, but not for sure. >>>> >>>> This should be fixed as well. >>> >>> Even worse, klp_init_func() can be called even if the object >>> hasn't been loaded. In that case there's no way to know what the >>> value of n is, and therefore no way to reliably create the sysfs >>> entry. >> >> Ah, right. >> >>> Should we create "func,n" in klp_init_object_loaded() instead of >>> klp_init_func()? >> >> So that the function entries in sysfs would be created only when >> the object is loaded? Well, why not, but in that case it could >> easily confuse the user. > > Maybe, but I think it would be fine if we document it. It should > only be relied on by tools, anyway. > >> Object entry would be empty for not loaded object. I would not dare >> to propose to remove such object entries. It would make things >> worse. > > Why would removing an empty object entry make things worse? > >> So maybe we could introduce an attribute in sysfs object entry >> which would say if the object is loaded or not. Or something like >> that. > > Hm, I'm not sure I see how this would help. > >> Hm, we can easily mess this up :) > > Agreed 100% :-) > Working on v3 with these suggestions. - Documentation fixes - create func,n in klp_init_object_loaded I'll test unique and non-unique functions being patched. In addition I'll test this when the object is vmlinux or a module (to test the object being loaded later). --chris