Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1270379 > unrolled thread

[PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

Started byChris J Arges <chris.j.arges@canonical.com>
First post2015-11-16 18:10 +0100
Last post2015-11-18 21:40 +0100
Articles 8 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-16 18:10 +0100
    Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Miroslav Benes <mbenes@suse.cz> - 2015-11-18 11:00 +0100
      Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-18 15:10 +0100
        Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-18 17:50 +0100
    Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-18 17:40 +0100
      Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-18 17:40 +0100
        Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-18 18:00 +0100
          Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to  klp_reloc Jiri Kosina <jikos@kernel.org> - 2015-11-18 21:40 +0100

#1270379 — [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-16 18:10 +0100
Subject[PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qvvrX-1EC-3@gated-at.bofh.it>
In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. By default 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.

Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
as they are no longer used.

From the klp_reloc structure remove val, as it can be refactored as a
local variable in klp_write_object_relocations.

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 include/linux/livepatch.h |  5 ++--
 kernel/livepatch/core.c   | 75 +++++++++++------------------------------------
 2 files changed, 19 insertions(+), 61 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index b60e8ab..a882865 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -67,8 +67,7 @@ struct klp_func {
 /**
  * struct klp_reloc - relocation structure for live patching
  * @loc:	address where the relocation will be written
- * @val:	address of the referenced symbol (optional,
- *		vmlinux	patches only)
+ * @sympos:	position in kallsyms to disambiguate symbols (optional)
  * @type:	ELF relocation type
  * @name:	name of the referenced symbol (for lookup/verification)
  * @addend:	offset from the referenced symbol
@@ -76,7 +75,7 @@ struct klp_func {
  */
 struct klp_reloc {
 	unsigned long loc;
-	unsigned long val;
+	unsigned long sympos;
 	unsigned long type;
 	const char *name;
 	int addend;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5a53383..9d98c7b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -203,45 +203,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-struct klp_verify_args {
-	const char *name;
-	const unsigned long addr;
-};
-
-static int klp_verify_callback(void *data, const char *name,
-			       struct module *mod, unsigned long addr)
-{
-	struct klp_verify_args *args = data;
-
-	if (!mod &&
-	    !strcmp(args->name, name) &&
-	    args->addr == addr)
-		return 1;
-
-	return 0;
-}
-
-static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
-{
-	struct klp_verify_args args = {
-		.name = name,
-		.addr = addr,
-	};
-	int ret;
-
-	mutex_lock(&module_mutex);
-	ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
-	mutex_unlock(&module_mutex);
-
-	if (!ret) {
-		pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
-			name, addr);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /*
  * external symbols are located outside the parent object (where the parent
  * object is either vmlinux or the kmod being patched).
@@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
 					struct klp_object *obj)
 {
 	int ret;
+	unsigned long val;
 	struct klp_reloc *reloc;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -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;
 		}
+		/* discover the address of the referenced symbol */
+		if (reloc->external) {
+			ret = klp_find_external_symbol(pmod, reloc->name, &val);
+		} else
+			ret = klp_find_object_symbol(obj->mod->name,
+						     reloc->name,
+						     reloc->sympos,
+						     &val);
+		if (ret)
+			return ret;
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
-					     reloc->val + reloc->addend);
+					     val + reloc->addend);
 		if (ret) {
 			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
-			       reloc->name, reloc->val, ret);
+			       reloc->name, val, ret);
 			return ret;
 		}
 	}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1272067 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-18 11:00 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qw7GW-1aT-23@gated-at.bofh.it>
In reply to#1270379
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.

Miroslav

> +		/* discover the address of the referenced symbol */
> +		if (reloc->external) {
> +			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> +		} else
> +			ret = klp_find_object_symbol(obj->mod->name,
> +						     reloc->name,
> +						     reloc->sympos,
> +						     &val);
> +		if (ret)
> +			return ret;
>  		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     reloc->val + reloc->addend);
> +					     val + reloc->addend);
>  		if (ret) {
>  			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, reloc->val, ret);
> +			       reloc->name, val, ret);
>  			return ret;
>  		}
>  	}
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272198 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-18 15:10 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwbAT-3W1-17@gated-at.bofh.it>
In reply to#1272067
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272378 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromPetr Mladek <pmladek@suse.com>
Date2015-11-18 17:50 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwe5I-5ul-15@gated-at.bofh.it>
In reply to#1272198
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272357 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromPetr Mladek <pmladek@suse.com>
Date2015-11-18 17:40 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwdW2-5qI-3@gated-at.bofh.it>
In reply to#1270379
On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default 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.
> 
> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> as they are no longer used.
> 
> >From the klp_reloc structure remove val, as it can be refactored as a
  ^
Please, remove this extra '>'. I guess that you cut&pasted the
comment from a mail reply ;-)

Best Regards,
Petr


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272365 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-18 17:40 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwdW3-5qI-21@gated-at.bofh.it>
In reply to#1272357
On 11/18/2015 10:37 AM, Petr Mladek wrote:
> On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
>> In cases of duplicate symbols, sympos will be used to disambiguate instead
>> of val. By default 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.
>>
>> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
>> as they are no longer used.
>>
>> >From the klp_reloc structure remove val, as it can be refactored as a
>   ^
> Please, remove this extra '>'. I guess that you cut&pasted the
> comment from a mail reply ;-)
> 
> Best Regards,
> Petr
> 

I think that '>' was added in a subsequent reply. My patch doesn't have
that, and I'm guessing checkpatch would have warned me about that as well.
: )

--chris

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272382 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromPetr Mladek <pmladek@suse.com>
Date2015-11-18 18:00 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwefo-5yL-19@gated-at.bofh.it>
In reply to#1272365
On Wed 2015-11-18 10:39:24, Chris J Arges wrote:
> On 11/18/2015 10:37 AM, Petr Mladek wrote:
> > On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
> >> In cases of duplicate symbols, sympos will be used to disambiguate instead
> >> of val. By default 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.
> >>
> >> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> >> as they are no longer used.
> >>
> >> >From the klp_reloc structure remove val, as it can be refactored as a
> >   ^
> > Please, remove this extra '>'. I guess that you cut&pasted the
> > comment from a mail reply ;-)
> > 
> > Best Regards,
> > Petr
> > 
> 
> I think that '>' was added in a subsequent reply. My patch doesn't have
> that, and I'm guessing checkpatch would have warned me about that as well.
> : )

Heh, it seems that some mail delivery tool or mail client tries to be
clever.  I see the '>' in you original mail. But it is fine in the
archive, see
http://article.gmane.org/gmane.linux.kernel/2086364
http://marc.info/?l=linux-kernel&m=144769343212421

I am sorry for the noise.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272551 — Re: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc

FromJiri Kosina <jikos@kernel.org>
Date2015-11-18 21:40 +0100
SubjectRe: [PATCH 2/3 v7] livepatch: add sympos as disambiguator field to klp_reloc
Message-ID<qwhGi-7Ze-17@gated-at.bofh.it>
In reply to#1272382
On Wed, 18 Nov 2015, Petr Mladek wrote:

> > I think that '>' was added in a subsequent reply. My patch doesn't have
> > that, and I'm guessing checkpatch would have warned me about that as well.
> > : )
> 
> Heh, it seems that some mail delivery tool or mail client tries to be
> clever.  I see the '>' in you original mail. But it is fine in the
> archive, see
> http://article.gmane.org/gmane.linux.kernel/2086364
> http://marc.info/?l=linux-kernel&m=144769343212421

It has to be stored this way if you are using mbox format as a storage, as 
lines startng with "From" that directly follow an empty line (which is 
exactly what happened here) have special meaning in mbox format (it is 
used as a message separator).

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web