From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933058AbbKMQ1K (ORCPT ); Fri, 13 Nov 2015 11:27:10 -0500 Received: from mx2.suse.de ([195.135.220.15]:46104 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932281AbbKMQ1H (ORCPT ); Fri, 13 Nov 2015 11:27:07 -0500 Date: Fri, 13 Nov 2015 17:27:05 +0100 From: Petr Mladek To: Chris J Arges Cc: live-patching@vger.kernel.org, jeyu@redhat.com, Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/4 v5] livepatch: add old_sympos as disambiguator field to klp_func Message-ID: <20151113162705.GW2599@pathway.suse.cz> References: <1447259366-7055-1-git-send-email-chris.j.arges@canonical.com> <1447347595-30728-1-git-send-email-chris.j.arges@canonical.com> <1447347595-30728-2-git-send-email-chris.j.arges@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447347595-30728-2-git-send-email-chris.j.arges@canonical.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 2015-11-12 10:59:50, Chris J Arges wrote: > In cases of duplicate symbols, old_sympos will be used to disambiguate > instead of old_addr. By default old_sympos will be 0, and patching will > only succeed if the symbol is unique. Specifying a positive value will > ensure that occurrence of the symbol in kallsyms for the patched object > will be used for patching if it is valid. > > In addition, make old_addr an internal structure field not to be specified > by the user. Finally, remove klp_find_verify_func_addr as it can be > replaced by klp_find_object_symbol directly. > > Signed-off-by: Chris J Arges > --- > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 6e53441..479d75e 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -159,36 +154,46 @@ static int klp_find_callback(void *data, const char *name, > return 0; > > /* > - * args->addr might be overwritten if another match is found > - * but klp_find_object_symbol() handles this and only returns the > - * addr if count == 1. > + * Increment and assign the address. Return 1 when the desired symbol > + * position is found or the search for a unique symbol fails. > */ > args->addr = addr; > args->count++; I would avoid the obvious things and move the comment here: /* * Finish the search when the symbol is found on the desired * position or the position is not defined for non-unique * symbol. */ > + if (((args->pos > 0) && (args->count == args->pos)) || > + ((args->pos == 0) && (args->count > 1))) > + return 1; I wonder if this is better readable: if ((args->pos && (args->count == args->pos)) || (!args->pos && (args->count > 1))) return 1; > return 0; > } > [...] > @@ -277,7 +261,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name, > preempt_enable(); > > /* otherwise check if it's in another .o within the patch module */ > - return klp_find_object_symbol(pmod->name, name, addr); > + return klp_find_object_symbol(pmod->name, name, addr, 0); Please, add a comment here that external symbols always have to be unique. > } > > static int klp_write_object_relocations(struct module *pmod, > @@ -307,7 +291,7 @@ static int klp_write_object_relocations(struct module *pmod, > else > ret = klp_find_object_symbol(obj->mod->name, > reloc->name, > - &reloc->val); > + &reloc->val, 0); Please, mention in the commit message that the support for sympos will be added into relocations in a separate patch. Or add a FIXME comment here. It will make easier the review and git history archaeology. Thank you, Petr