From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756388AbbKRQrN (ORCPT ); Wed, 18 Nov 2015 11:47:13 -0500 Received: from mx2.suse.de ([195.135.220.15]:53392 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756308AbbKRQrL (ORCPT ); Wed, 18 Nov 2015 11:47:11 -0500 Date: Wed, 18 Nov 2015 17:47:09 +0100 From: Petr Mladek To: Josh Poimboeuf Cc: Miroslav Benes , Chris J Arges , live-patching@vger.kernel.org, sjenning@redhat.com, jikos@kernel.org, vojtech@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: <20151118164709.GN4431@pathway.suse.cz> 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> <20151118140137.GA3620@treble.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151118140137.GA3620@treble.redhat.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 Wed 2015-11-18 08:01:37, Josh Poimboeuf wrote: > 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. OK, I do not have a strong opinion here. I am fain with refusing any non-zero value. Just please improve the error message. If I find in dmesg "symbol 'bla' is external and has sympos 1" I would worry what the problem is. Please, somehow express that sympos is not supported for external relocated symbols. You are not limited by the 80 characters here! Best Regards, Petr