From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755924AbbKROBp (ORCPT ); Wed, 18 Nov 2015 09:01:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34494 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932085AbbKROBj (ORCPT ); Wed, 18 Nov 2015 09:01:39 -0500 Date: Wed, 18 Nov 2015 08:01:37 -0600 From: Josh Poimboeuf To: Miroslav Benes Cc: Chris J Arges , live-patching@vger.kernel.org, sjenning@redhat.com, jikos@kernel.org, vojtech@suse.com, pmladek@suse.com, jeyu@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc Message-ID: <20151118140137.GA3620@treble.redhat.com> References: <1447431804-18786-1-git-send-email-chris.j.arges@canonical.com> <1447693391-10065-1-git-send-email-chris.j.arges@canonical.com> <1447693391-10065-3-git-send-email-chris.j.arges@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 Wed, Nov 18, 2015 at 10:56:00AM +0100, Miroslav Benes wrote: > On Mon, 16 Nov 2015, Chris J Arges wrote: > > > @@ -281,29 +243,26 @@ static int klp_write_object_relocations(struct module *pmod, > > return -EINVAL; > > > > for (reloc = obj->relocs; reloc->name; reloc++) { > > - if (!klp_is_module(obj)) { > > - ret = klp_verify_vmlinux_symbol(reloc->name, > > - reloc->val); > > - if (ret) > > - return ret; > > - } else { > > - /* module, reloc->val needs to be discovered */ > > - if (reloc->external) > > - ret = klp_find_external_symbol(pmod, > > - reloc->name, > > - &reloc->val); > > - else > > - ret = klp_find_object_symbol(obj->mod->name, > > - reloc->name, > > - 0, &reloc->val); > > - if (ret) > > - return ret; > > + if (reloc->sympos && (reloc->sympos != 1)) { > > + pr_err("symbol '%s' is external and has sympos %lu\n", > > + reloc->name, reloc->sympos); > > + return -EINVAL; > > } > > Oh, this check should not be here, right? I think it needs to be under 'if > (reloc->external) {'. > > And > > if (reloc->sympos && (reloc->sympos != 1)) { > > can be written as > > if (reloc->sympos > 1) { > > Anyway, I am not sure if this is the right thing to do. I know Petr > suggested it, but maybe it would be sufficient just to error out when > reloc->sympos is specified for external symbols. Despite there is a valid > value (== 1). Like it had been done before. Josh came up with the issue. > So what is your opinion, Josh? It is a nitpick, but nevertheless. I tend to agree. IMO, any non-zero value of sympos doesn't make sense for external symbols. -- Josh