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

Groups > linux.kernel > #1260841 > unrolled thread

[PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

Started byChris J Arges <chris.j.arges@canonical.com>
First post2015-11-02 19:00 +0100
Last post2015-11-10 10:10 +0100
Articles 20 on this page of 43 — 6 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 19:00 +0100
    Re: livepatch: old_name.number scheme in livepatch sysfs directory Jessica Yu <jeyu@redhat.com> - 2015-11-02 20:20 +0100
    Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:00 +0100
      Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 21:20 +0100
        Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:40 +0100
          [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 00:10 +0100
            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 11:00 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 12:00 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Petr Mladek <pmladek@suse.com> - 2015-11-03 13:50 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Jiri Kosina <jikos@kernel.org> - 2015-11-03 21:00 +0100
                  Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 21:10 +0100
                    [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:30 +0100
                      Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field  to klp_func Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 18:50 +0100
                      Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field  to klp_func Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
                    [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                      Re: [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch  sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:10 +0100
                    Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                    [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                      Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:00 +0100
                        Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-12 15:40 +0100
                          Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 20:20 +0100
                            Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-13 15:00 +0100
                              Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-13 18:00 +0100
                      Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:00 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 17:20 +0100
                  Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 18:00 +0100
                    Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 21:50 +0100
                    Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-04 11:00 +0100
                      Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-04 17:10 +0100
                        Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-04 17:20 +0100
                        Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-05 16:20 +0100
                          Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 17:00 +0100
                            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-05 17:10 +0100
                            [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-09 17:20 +0100
                              Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-09 22:00 +0100
                                Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-10 00:10 +0100
                                  Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 06:00 +0100
                                    Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 09:50 +0100
                                      Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 14:50 +0100
                              Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 10:10 +0100

Page 1 of 3  [1] 2 3  Next page →

#1260841 — [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-02 19:00 +0100
Subject[PATCH] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqryF-8gz-1@gated-at.bofh.it>
The following directory structure will allow for cases when the same
function name exists in a single object.
	/sys/kernel/livepatch/<patch>/<object>/<function.number>

The number is incremented on each known initialized func kobj thus creating
unique names in this case.

An example of this issue is documented here:
	https://github.com/dynup/kpatch/issues/493

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
 kernel/livepatch/core.c                          | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..dcd36db 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
 		The object directory contains subdirectories for each function
 		that is patched within the object.
 
-What:		/sys/kernel/livepatch/<patch>/<object>/<function>
+What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>
 Date:		Nov 2014
 KernelVersion:	3.19.0
 Contact:	live-patching@vger.kernel.org
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..ecacf65 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<func.number>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -727,13 +727,29 @@ static void klp_free_patch(struct klp_patch *patch)
 	kobject_put(&patch->kobj);
 }
 
+static int klp_count_sysfs_funcs(struct klp_object *obj, const char *name)
+{
+	struct klp_func *func;
+	int n = 0;
+
+	/* count the times a function name occurs and is initialized */
+	klp_for_each_func(obj, func) {
+		if ((!strcmp(func->old_name, name) &&
+		    func->kobj.state_initialized))
+			n++;
+	}
+
+	return n;
+}
+
 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_count_sysfs_funcs(obj, func->old_name));
 }
 
 /* parts of the initialization that is done only when the object is loaded */
-- 
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]

#1260895 — Re: livepatch: old_name.number scheme in livepatch sysfs directory

FromJessica Yu <jeyu@redhat.com>
Date2015-11-02 20:20 +0100
SubjectRe: livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqsO5-JS-9@gated-at.bofh.it>
In reply to#1260841
+++ Chris J Arges [02/11/15 11:58 -0600]:
>The following directory structure will allow for cases when the same
>function name exists in a single object.
>	/sys/kernel/livepatch/<patch>/<object>/<function.number>
>
>The number is incremented on each known initialized func kobj thus creating
>unique names in this case.
>
>An example of this issue is documented here:
>	https://github.com/dynup/kpatch/issues/493
>
>Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>

Thanks Chris. Verified that the patch fixes the panic caused by
multiple functions with the same name and object.

Acked-by: Jessica Yu <jeyu@redhat.com>
--
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]

#1260919 — Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-02 21:00 +0100
SubjectRe: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqtqP-10k-19@gated-at.bofh.it>
In reply to#1260841
On Mon, Nov 02, 2015 at 11:58:47AM -0600, Chris J Arges wrote:
> The following directory structure will allow for cases when the same
> function name exists in a single object.
> 	/sys/kernel/livepatch/<patch>/<object>/<function.number>
> 
> The number is incremented on each known initialized func kobj thus creating
> unique names in this case.
> 
> An example of this issue is documented here:
> 	https://github.com/dynup/kpatch/issues/493
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
>  kernel/livepatch/core.c                          | 20 ++++++++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index 5bf42a8..dcd36db 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -33,7 +33,7 @@ Description:
>  		The object directory contains subdirectories for each function
>  		that is patched within the object.
>  
> -What:		/sys/kernel/livepatch/<patch>/<object>/<function>
> +What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
>  Contact:	live-patching@vger.kernel.org
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..ecacf65 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/<object>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>
> + * /sys/kernel/livepatch/<patch>/<object>/<func.number>
>   */
>  
>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -727,13 +727,29 @@ static void klp_free_patch(struct klp_patch *patch)
>  	kobject_put(&patch->kobj);
>  }
>  
> +static int klp_count_sysfs_funcs(struct klp_object *obj, const char *name)
> +{
> +	struct klp_func *func;
> +	int n = 0;
> +
> +	/* count the times a function name occurs and is initialized */
> +	klp_for_each_func(obj, func) {
> +		if ((!strcmp(func->old_name, name) &&
> +		    func->kobj.state_initialized))
> +			n++;
> +	}
> +
> +	return n;
> +}
> +
>  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_count_sysfs_funcs(obj, func->old_name));
>  }
>  
>  /* parts of the initialization that is done only when the object is loaded */
> -- 
> 1.9.1

I'd prefer something other than a period for the string separator
because some symbols have a period in their name.  How about a space?

Also, this shows the nth occurrence of the symbol name in the patch
module.  But I think it would be better to instead display the nth
occurrence of the symbol name in the kallsyms for the patched object.
That way user space can deterministically detect which function was
patched.

For example:

  $ grep " t_next" /proc/kallsyms
  ffffffff811597d0 t t_next
  ffffffff81163bb0 t t_next
  ...

In my kernel there are 6 functions named t_next in vmlinux.  "t_next 0"
would refer to the function at 0xffffffff811597d0.  "t_next 1" would
refer to the one at 0xffffffff81163bb0.

While we're at it, should we also encode the replacement function name
(func->new_func)?  e.g.:

  "t_next 0 t_next__patched".


-- 
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]

#1260935 — Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-02 21:20 +0100
SubjectRe: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqtKa-1nc-17@gated-at.bofh.it>
In reply to#1260919
On Mon, Nov 02, 2015 at 01:52:44PM -0600, Josh Poimboeuf wrote:
> On Mon, Nov 02, 2015 at 11:58:47AM -0600, Chris J Arges wrote:
> > The following directory structure will allow for cases when the same
> > function name exists in a single object.
> > 	/sys/kernel/livepatch/<patch>/<object>/<function.number>
> > 
> > The number is incremented on each known initialized func kobj thus creating
> > unique names in this case.
> > 
> > An example of this issue is documented here:
> > 	https://github.com/dynup/kpatch/issues/493
> > 
> > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> > ---
> >  Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
> >  kernel/livepatch/core.c                          | 20 ++++++++++++++++++--
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> > index 5bf42a8..dcd36db 100644
> > --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> > +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> > @@ -33,7 +33,7 @@ Description:
> >  		The object directory contains subdirectories for each function
> >  		that is patched within the object.
> >  
> > -What:		/sys/kernel/livepatch/<patch>/<object>/<function>
> > +What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>
> >  Date:		Nov 2014
> >  KernelVersion:	3.19.0
> >  Contact:	live-patching@vger.kernel.org
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 6e53441..ecacf65 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
> >   * /sys/kernel/livepatch/<patch>
> >   * /sys/kernel/livepatch/<patch>/enabled
> >   * /sys/kernel/livepatch/<patch>/<object>
> > - * /sys/kernel/livepatch/<patch>/<object>/<func>
> > + * /sys/kernel/livepatch/<patch>/<object>/<func.number>
> >   */
> >  
> >  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> > @@ -727,13 +727,29 @@ static void klp_free_patch(struct klp_patch *patch)
> >  	kobject_put(&patch->kobj);
> >  }
> >  
> > +static int klp_count_sysfs_funcs(struct klp_object *obj, const char *name)
> > +{
> > +	struct klp_func *func;
> > +	int n = 0;
> > +
> > +	/* count the times a function name occurs and is initialized */
> > +	klp_for_each_func(obj, func) {
> > +		if ((!strcmp(func->old_name, name) &&
> > +		    func->kobj.state_initialized))
> > +			n++;
> > +	}
> > +
> > +	return n;
> > +}
> > +
> >  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_count_sysfs_funcs(obj, func->old_name));
> >  }
> >  
> >  /* parts of the initialization that is done only when the object is loaded */
> > -- 
> > 1.9.1
> 
> I'd prefer something other than a period for the string separator
> because some symbols have a period in their name.  How about a space?
> 

Perhaps a '-' would be better?
/t_next-0
/t_next-1

> Also, this shows the nth occurrence of the symbol name in the patch
> module.  But I think it would be better to instead display the nth
> occurrence of the symbol name in the kallsyms for the patched object.
> That way user space can deterministically detect which function was
> patched.
> 
> For example:
> 
>   $ grep " t_next" /proc/kallsyms
>   ffffffff811597d0 t t_next
>   ffffffff81163bb0 t t_next
>   ...
> 
> In my kernel there are 6 functions named t_next in vmlinux.  "t_next 0"
> would refer to the function at 0xffffffff811597d0.  "t_next 1" would
> refer to the one at 0xffffffff81163bb0.
> 

This makes sense to me.

> While we're at it, should we also encode the replacement function name
> (func->new_func)?  e.g.:
> 
>   "t_next 0 t_next__patched".
> 
> 
> -- 
> Josh
>

Since we are creating a directory for this function, at some point would we add
this as a file in that func directory? I think encoding the func name with
old_name + occurrence should accomplish uniqueness and consistency.

However, another approach would be:

/<old_func>-<new_func>

Which presumably would be unique and consistent (depending on how the patch was
authored).

--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]

#1260954 — Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-02 21:40 +0100
SubjectRe: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqu3w-1uH-23@gated-at.bofh.it>
In reply to#1260935
On Mon, Nov 02, 2015 at 02:16:16PM -0600, Chris J Arges wrote:
> On Mon, Nov 02, 2015 at 01:52:44PM -0600, Josh Poimboeuf wrote:
> > On Mon, Nov 02, 2015 at 11:58:47AM -0600, Chris J Arges wrote:
> > > The following directory structure will allow for cases when the same
> > > function name exists in a single object.
> > > 	/sys/kernel/livepatch/<patch>/<object>/<function.number>
> > > 
> > > The number is incremented on each known initialized func kobj thus creating
> > > unique names in this case.
> > > 
> > > An example of this issue is documented here:
> > > 	https://github.com/dynup/kpatch/issues/493
> > > 
> > > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
> > >  kernel/livepatch/core.c                          | 20 ++++++++++++++++++--
> > >  2 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> > > index 5bf42a8..dcd36db 100644
> > > --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> > > +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> > > @@ -33,7 +33,7 @@ Description:
> > >  		The object directory contains subdirectories for each function
> > >  		that is patched within the object.
> > >  
> > > -What:		/sys/kernel/livepatch/<patch>/<object>/<function>
> > > +What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>
> > >  Date:		Nov 2014
> > >  KernelVersion:	3.19.0
> > >  Contact:	live-patching@vger.kernel.org
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 6e53441..ecacf65 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
> > >   * /sys/kernel/livepatch/<patch>
> > >   * /sys/kernel/livepatch/<patch>/enabled
> > >   * /sys/kernel/livepatch/<patch>/<object>
> > > - * /sys/kernel/livepatch/<patch>/<object>/<func>
> > > + * /sys/kernel/livepatch/<patch>/<object>/<func.number>
> > >   */
> > >  
> > >  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > @@ -727,13 +727,29 @@ static void klp_free_patch(struct klp_patch *patch)
> > >  	kobject_put(&patch->kobj);
> > >  }
> > >  
> > > +static int klp_count_sysfs_funcs(struct klp_object *obj, const char *name)
> > > +{
> > > +	struct klp_func *func;
> > > +	int n = 0;
> > > +
> > > +	/* count the times a function name occurs and is initialized */
> > > +	klp_for_each_func(obj, func) {
> > > +		if ((!strcmp(func->old_name, name) &&
> > > +		    func->kobj.state_initialized))
> > > +			n++;
> > > +	}
> > > +
> > > +	return n;
> > > +}
> > > +
> > >  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_count_sysfs_funcs(obj, func->old_name));
> > >  }
> > >  
> > >  /* parts of the initialization that is done only when the object is loaded */
> > > -- 
> > > 1.9.1
> > 
> > I'd prefer something other than a period for the string separator
> > because some symbols have a period in their name.  How about a space?
> > 
> 
> Perhaps a '-' would be better?
> /t_next-0
> /t_next-1

Yeah, that would work.  I tend to prefer a space or a comma as a
delimiter but anything unique is fine.

> > Also, this shows the nth occurrence of the symbol name in the patch
> > module.  But I think it would be better to instead display the nth
> > occurrence of the symbol name in the kallsyms for the patched object.
> > That way user space can deterministically detect which function was
> > patched.
> > 
> > For example:
> > 
> >   $ grep " t_next" /proc/kallsyms
> >   ffffffff811597d0 t t_next
> >   ffffffff81163bb0 t t_next
> >   ...
> > 
> > In my kernel there are 6 functions named t_next in vmlinux.  "t_next 0"
> > would refer to the function at 0xffffffff811597d0.  "t_next 1" would
> > refer to the one at 0xffffffff81163bb0.
> > 
> 
> This makes sense to me.
> 
> > While we're at it, should we also encode the replacement function name
> > (func->new_func)?  e.g.:
> > 
> >   "t_next 0 t_next__patched".
> > 
> > 
> > -- 
> > Josh
> >
> 
> Since we are creating a directory for this function, at some point would we add
> this as a file in that func directory?

Yeah, we could always add that later.  It's probably best to wait until
somebody actually needs it anyway.

> I think encoding the func name with
> old_name + occurrence should accomplish uniqueness and consistency.
>
> However, another approach would be:
> 
> /<old_func>-<new_func>
> 
> Which presumably would be unique and consistent (depending on how the patch was
> authored).

As you said, depending on how the patch was authored, I think it would
still be possible for it to be not unique, as we can have duplicate
symbol names in the patch module too.

And also you wouldn't have the ability to determine exactly which
"old_func" is being patched, which could potentially be an important
detail.

-- 
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]

#1261047 — [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-03 00:10 +0100
Subject[PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqwoF-34I-7@gated-at.bofh.it>
In reply to#1260954
The following directory structure will allow for cases when the same
function name exists in a single object.
	/sys/kernel/livepatch/<patch>/<object>/<function.number>

The number corresponds to the nth occurrence of the symbol name in
kallsyms for the patched object.

An example of this issue is documented here:
	https://github.com/dynup/kpatch/issues/493

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
 kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..dcd36db 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
 		The object directory contains subdirectories for each function
 		that is patched within the object.
 
-What:		/sys/kernel/livepatch/<patch>/<object>/<function>
+What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>
 Date:		Nov 2014
 KernelVersion:	3.19.0
 Contact:	live-patching@vger.kernel.org
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..6bcf600 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<func.number>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -727,13 +727,54 @@ static void klp_free_patch(struct klp_patch *patch)
 	kobject_put(&patch->kobj);
 }
 
+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));
 }
 
 /* parts of the initialization that is done only when the object is loaded */
-- 
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] | [prev] | [next] | [standalone]

#1261353 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-03 11:00 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqGxI-10C-11@gated-at.bofh.it>
In reply to#1261047
On Mon, 2 Nov 2015, Chris J Arges wrote:

> The following directory structure will allow for cases when the same
> function name exists in a single object.
> 	/sys/kernel/livepatch/<patch>/<object>/<function.number>

There is still a period here and in the documentation :)

> The number corresponds to the nth occurrence of the symbol name in
> kallsyms for the patched object.
> 
> An example of this issue is documented here:
> 	https://github.com/dynup/kpatch/issues/493
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  2 +-
>  kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index 5bf42a8..dcd36db 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -33,7 +33,7 @@ Description:
>  		The object directory contains subdirectories for each function
>  		that is patched within the object.
>  
> -What:		/sys/kernel/livepatch/<patch>/<object>/<function>
> +What:		/sys/kernel/livepatch/<patch>/<object>/<function.number>

Dash should be here.

Since it is a documentation, could you add few words what this number is? 
I think that the sentence "The number corresponds..." from the changelog 
above would be great.

>  Date:		Nov 2014
>  KernelVersion:	3.19.0
>  Contact:	live-patching@vger.kernel.org
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..6bcf600 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/<object>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>
> + * /sys/kernel/livepatch/<patch>/<object>/<func.number>

And here.

The rest is fine.

Thanks a lot for the patch
Miroslav
--
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]

#1261564 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-03 16:10 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqLnI-4ku-19@gated-at.bofh.it>
In reply to#1261353
On Tue, Nov 03, 2015 at 10:50:05AM +0100, Miroslav Benes wrote:
> On Mon, 2 Nov 2015, Chris J Arges wrote:
> 
> > The following directory structure will allow for cases when the same
> > function name exists in a single object.
> > 	/sys/kernel/livepatch/<patch>/<object>/<function.number>
> 
> There is still a period here and in the documentation :)

and in the subject :-)

-- 
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]

#1261385 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-03 12:00 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqHtL-1CF-11@gated-at.bofh.it>
In reply to#1261047
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.

Miroslav
--
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]

#1261454 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromPetr Mladek <pmladek@suse.com>
Date2015-11-03 13:50 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqJce-2KO-5@gated-at.bofh.it>
In reply to#1261385
On Tue 2015-11-03 11:52:08, 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 might happen when the function name is unique. Then we might but
we do not need to pre-define the address in the patch.

Also I would omit the suffix at all when it is the first occurrence.
It will cause that unique symbols will not be numbered.

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]

#1261563 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-03 16:10 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqLnI-4ku-15@gated-at.bofh.it>
In reply to#1261454
On Tue, Nov 03, 2015 at 01:44:41PM +0100, Petr Mladek wrote:
> Also I would omit the suffix at all when it is the first occurrence.
> It will cause that unique symbols will not be numbered.

That would make parsing the entry unnecessarily harder and more
error-prone.  I think it should always have the suffix, so it's
consistent and there are no surprises.

-- 
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]

#1261831 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromJiri Kosina <jikos@kernel.org>
Date2015-11-03 21:00 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqPUm-79Z-15@gated-at.bofh.it>
In reply to#1261454
On Tue, 3 Nov 2015, Petr Mladek wrote:

> Also I would omit the suffix at all when it is the first occurrence. It 
> will cause that unique symbols will not be numbered.

That'd mean that the names (including suffixes) are not stable, because a 
particular name that has originally been unique can later be made 
non-unique when module brings in a conflicting name.

-- 
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] | [next] | [standalone]

#1261842 — Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-03 21:10 +0100
SubjectRe: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory
Message-ID<qqQ42-7sq-9@gated-at.bofh.it>
In reply to#1261831
On Tue, Nov 03, 2015 at 08:57:24PM +0100, Jiri Kosina wrote:
> On Tue, 3 Nov 2015, Petr Mladek wrote:
> 
> > Also I would omit the suffix at all when it is the first occurrence. It 
> > will cause that unique symbols will not be numbered.
> 
> That'd mean that the names (including suffixes) are not stable, because a 
> particular name that has originally been unique can later be made 
> non-unique when module brings in a conflicting name.

The numbering (and uniqueness) is per-object, so the same symbol name
from another module would live in a separate namespace and wouldn't
create a conflict.

-- 
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]

#1267288 — [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-11 17:30 +0100
Subject[PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func
Message-ID<qtGrw-592-5@gated-at.bofh.it>
In reply to#1261842
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 will be used for patching if it is
valid. Finally, old_addr is now an internal structure element and not to
be specified by the user.

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..df7b752 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,8 +37,9 @@ enum klp_state {
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
  * @new_func:	pointer to the patched function code
- * @old_addr:	a hint conveying at what address the old function
- *		can be found (optional, vmlinux patches only)
+ * @old_sympos: a hint indicating which symbol position the old function
+ *		can be found (optional)
+ * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @state:	tracks function-level patch application state
  * @stack_node:	list node for klp_ops func_stack list
@@ -47,17 +48,18 @@ struct klp_func {
 	/* external */
 	const char *old_name;
 	void *new_func;
+
 	/*
-	 * The old_addr field is optional and can be used to resolve
-	 * duplicate symbol names in the vmlinux object.  If this
-	 * information is not present, the symbol is located by name
-	 * with kallsyms. If the name is not unique and old_addr is
-	 * not provided, the patch application fails as there is no
-	 * way to resolve the ambiguity.
+	 * The old_sympos field is optional and can be used to resolve
+	 * duplicate symbol names in livepatch objects. If this field is zero,
+	 * it is expected the symbol is unique, otherwise patching fails. If
+	 * this value is greater than zero then that occurrence of the symbol
+	 * in kallsyms is used.
 	 */
-	unsigned long old_addr;
+	unsigned long old_sympos;
 
 	/* internal */
+	unsigned long old_addr;
 	struct kobject kobj;
 	enum klp_state state;
 	struct list_head stack_node;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..26f9778 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -142,6 +142,7 @@ struct klp_find_arg {
 	 * name in the same object.
 	 */
 	unsigned long count;
+	unsigned long pos;
 };
 
 static int klp_find_callback(void *data, const char *name,
@@ -159,36 +160,45 @@ 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 address, return only if checking pos and
+	 * it matches count.
 	 */
-	args->addr = addr;
 	args->count++;
+	args->addr = addr;
+	if ((args->pos > 0) && (args->count == args->pos))
+		return 1;
 
 	return 0;
 }
 
 static int klp_find_object_symbol(const char *objname, const char *name,
-				  unsigned long *addr)
+				  unsigned long *addr, unsigned long sympos)
 {
 	struct klp_find_arg args = {
 		.objname = objname,
 		.name = name,
 		.addr = 0,
-		.count = 0
+		.count = 0,
+		.pos = sympos,
 	};
 
 	mutex_lock(&module_mutex);
 	kallsyms_on_each_symbol(klp_find_callback, &args);
 	mutex_unlock(&module_mutex);
 
-	if (args.count == 0)
+	/*
+	 * Ensure an address was found. If sympos is 0, ensure symbol is unique;
+	 * otherwise ensure the symbol position count matches sympos.
+	 */
+	if (args.addr == 0)
 		pr_err("symbol '%s' not found in symbol table\n", name);
-	else if (args.count > 1)
+	else if (args.count > 1 && sympos == 0) {
 		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
 		       args.count, name, objname);
-	else {
+	} else if (sympos != args.count && sympos > 0) {
+		pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
+		       sympos, name, objname ? objname : "vmlinux");
+	} else {
 		*addr = args.addr;
 		return 0;
 	}
@@ -239,22 +249,11 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
 static int klp_find_verify_func_addr(struct klp_object *obj,
 				     struct klp_func *func)
 {
-	int ret;
-
-#if defined(CONFIG_RANDOMIZE_BASE)
-	/* If KASLR has been enabled, adjust old_addr accordingly */
-	if (kaslr_enabled() && func->old_addr)
-		func->old_addr += kaslr_offset();
-#endif
-
-	if (!func->old_addr || klp_is_module(obj))
-		ret = klp_find_object_symbol(obj->name, func->old_name,
-					     &func->old_addr);
-	else
-		ret = klp_verify_vmlinux_symbol(func->old_name,
-						func->old_addr);
-
-	return ret;
+	/*
+	 * Verify the symbol, find old_addr, and write it to the structure.
+	 */
+	return klp_find_object_symbol(obj->name, func->old_name,
+				      &func->old_addr, func->old_sympos);
 }
 
 /*
@@ -277,7 +276,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);
 }
 
 static int klp_write_object_relocations(struct module *pmod,
@@ -307,7 +306,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);
 			if (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] | [prev] | [next] | [standalone]

#1267344 — Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-11 18:50 +0100
SubjectRe: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func
Message-ID<qtHGV-5Rq-1@gated-at.bofh.it>
In reply to#1267288
On Wed, Nov 11, 2015 at 10:28:59AM -0600, 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 will be used for patching if it is
> valid. Finally, old_addr is now an internal structure element and not to
> be specified by the user.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  include/linux/livepatch.h | 20 ++++++++++--------
>  kernel/livepatch/core.c   | 53 +++++++++++++++++++++++------------------------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..df7b752 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -37,8 +37,9 @@ enum klp_state {
>   * struct klp_func - function structure for live patching
>   * @old_name:	name of the function to be patched
>   * @new_func:	pointer to the patched function code
> - * @old_addr:	a hint conveying at what address the old function
> - *		can be found (optional, vmlinux patches only)
> + * @old_sympos: a hint indicating which symbol position the old function
> + *		can be found (optional)
> + * @old_addr:	the address of the function being patched
>   * @kobj:	kobject for sysfs resources
>   * @state:	tracks function-level patch application state
>   * @stack_node:	list node for klp_ops func_stack list
> @@ -47,17 +48,18 @@ struct klp_func {
>  	/* external */
>  	const char *old_name;
>  	void *new_func;
> +
>  	/*
> -	 * The old_addr field is optional and can be used to resolve
> -	 * duplicate symbol names in the vmlinux object.  If this
> -	 * information is not present, the symbol is located by name
> -	 * with kallsyms. If the name is not unique and old_addr is
> -	 * not provided, the patch application fails as there is no
> -	 * way to resolve the ambiguity.
> +	 * The old_sympos field is optional and can be used to resolve
> +	 * duplicate symbol names in livepatch objects. If this field is zero,
> +	 * it is expected the symbol is unique, otherwise patching fails. If
> +	 * this value is greater than zero then that occurrence of the symbol
> +	 * in kallsyms is used.

I would clarify this:

...occurrence of the symbol in kallsyms *for the given object* is used.

>  	 */
> -	unsigned long old_addr;
> +	unsigned long old_sympos;
>  
>  	/* internal */
> +	unsigned long old_addr;
>  	struct kobject kobj;
>  	enum klp_state state;
>  	struct list_head stack_node;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..26f9778 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,7 @@ struct klp_find_arg {
>  	 * name in the same object.
>  	 */
>  	unsigned long count;
> +	unsigned long pos;

There's a comment above this that says:

	/*
	 * If count == 0, the symbol was not found. If count == 1, a unique
	 * match was found and addr is set.  If count > 1, there is
	 * unresolvable ambiguity among "count" number of symbols with the same
	 * name in the same object.
	 */

That comment is no longer accurate and can probably be removed since IMO
the purpose of 'count' is obvious.

>  };
>  
>  static int klp_find_callback(void *data, const char *name,
> @@ -159,36 +160,45 @@ 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 address, return only if checking pos and
> +	 * it matches count.
>  	 */
> -	args->addr = addr;
>  	args->count++;
> +	args->addr = addr;
> +	if ((args->pos > 0) && (args->count == args->pos))
> +		return 1;
>  
>  	return 0;
>  }
>  
>  static int klp_find_object_symbol(const char *objname, const char *name,
> -				  unsigned long *addr)
> +				  unsigned long *addr, unsigned long sympos)
>  {
>  	struct klp_find_arg args = {
>  		.objname = objname,
>  		.name = name,
>  		.addr = 0,
> -		.count = 0
> +		.count = 0,
> +		.pos = sympos,
>  	};
>  
>  	mutex_lock(&module_mutex);
>  	kallsyms_on_each_symbol(klp_find_callback, &args);
>  	mutex_unlock(&module_mutex);
>  
> -	if (args.count == 0)
> +	/*
> +	 * Ensure an address was found. If sympos is 0, ensure symbol is unique;
> +	 * otherwise ensure the symbol position count matches sympos.
> +	 */
> +	if (args.addr == 0)
>  		pr_err("symbol '%s' not found in symbol table\n", name);
> -	else if (args.count > 1)
> +	else if (args.count > 1 && sympos == 0) {
>  		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
>  		       args.count, name, objname);
> -	else {
> +	} else if (sympos != args.count && sympos > 0) {
> +		pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
> +		       sympos, name, objname ? objname : "vmlinux");
> +	} else {
>  		*addr = args.addr;
>  		return 0;
>  	}
> @@ -239,22 +249,11 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
>  static int klp_find_verify_func_addr(struct klp_object *obj,
>  				     struct klp_func *func)
>  {
> -	int ret;
> -
> -#if defined(CONFIG_RANDOMIZE_BASE)
> -	/* If KASLR has been enabled, adjust old_addr accordingly */
> -	if (kaslr_enabled() && func->old_addr)
> -		func->old_addr += kaslr_offset();
> -#endif
> -
> -	if (!func->old_addr || klp_is_module(obj))
> -		ret = klp_find_object_symbol(obj->name, func->old_name,
> -					     &func->old_addr);
> -	else
> -		ret = klp_verify_vmlinux_symbol(func->old_name,
> -						func->old_addr);
> -
> -	return ret;
> +	/*
> +	 * Verify the symbol, find old_addr, and write it to the structure.
> +	 */
> +	return klp_find_object_symbol(obj->name, func->old_name,
> +				      &func->old_addr, func->old_sympos);

klp_find_verify_func_addr() is no longer correctly named and can
probably be removed since klp_init_object_loaded() can call
klp_find_object_symbol() directly.

>  }
>  
>  /*
> @@ -277,7 +276,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);
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +306,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);
>  			if (ret)
>  				return ret;
>  		}
> -- 
> 1.9.1
> 

-- 
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]

#1267796 — Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-12 11:30 +0100
SubjectRe: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func
Message-ID<qtXiH-7Fc-33@gated-at.bofh.it>
In reply to#1267288
Next to Josh's remarks I have some more (mainly nitpicks, so it is often 
up to you).

On Wed, 11 Nov 2015, 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 will be used for patching if it is

"...occurrence of the symbol in kallsyms for the patched object will 
be used..."

Just to have it even in the changelog for clarity.

> valid. Finally, old_addr is now an internal structure element and not to
> be specified by the user.


> @@ -159,36 +160,45 @@ 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 address, return only if checking pos and
> +	 * it matches count.
>  	 */
> -	args->addr = addr;
>  	args->count++;
> +	args->addr = addr;

I guess that this row swap is remnant of a rebase. Anyway it is 
superfluous.

> +	if ((args->pos > 0) && (args->count == args->pos))
> +		return 1;

We could add an optimization here. If args->pos == 0 and args->count > 1 
we can return 1, because the symbol is not unique. The case is then 
correctly handled in klp_find_object_symbol. There is no need to walk 
through the rest of kallsyms.

Thanks,
Miroslav
--
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]

#1267301 — [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-11 17:40 +0100
Subject[PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory
Message-ID<qtGBc-5cy-11@gated-at.bofh.it>
In reply to#1261842
The following directory structure will allow for cases when the same
function name exists in a single object.
	/sys/kernel/livepatch/<patch>/<object>/<function,number>

The number corresponds to the nth occurrence of the symbol name in
kallsyms for the patched object.

An example of patching multiple symbols can be found here:
	https://github.com/dynup/kpatch/issues/493

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  6 +++++-
 kernel/livepatch/core.c                          | 10 ++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..21b6bc1 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
 		The object directory contains subdirectories for each function
 		that is patched within the object.
 
-What:		/sys/kernel/livepatch/<patch>/<object>/<function>
+What:		/sys/kernel/livepatch/<patch>/<object>/<function,number>
 Date:		Nov 2014
 KernelVersion:	3.19.0
 Contact:	live-patching@vger.kernel.org
@@ -41,4 +41,8 @@ Description:
 		The function directory contains attributes regarding the
 		properties and state of the patched function.
 
+		The directory name contains the patched function name and a
+		number corresponding to the nth occurrence of the symbol name
+		in kallsyms for the patched object.
+
 		There are currently no such attributes.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 4eb8691..ed2cbbf 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<func,number>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -687,8 +687,14 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 	INIT_LIST_HEAD(&func->stack_node);
 	func->state = KLP_DISABLED;
 
+	/* The format for the sysfs directory is <func,number> where number is
+	 * the occurrence of this symbol in kallsyms. If the user selects 0 for
+	 * old_sympos, then 1 will be used since a unique symbol will be the
+	 * first occurrence.
+	 */
 	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
-				    &obj->kobj, "%s", func->old_name);
+				    &obj->kobj, "%s,%lu", func->old_name,
+				    func->old_sympos ? func->old_sympos : 1);
 }
 
 /* parts of the initialization that is done only when the object is loaded */
-- 
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] | [prev] | [next] | [standalone]

#1267358 — Re: [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-11 19:10 +0100
SubjectRe: [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory
Message-ID<qtI0h-6el-5@gated-at.bofh.it>
In reply to#1267301
On Wed, Nov 11, 2015 at 10:29:01AM -0600, Chris J Arges wrote:
> The following directory structure will allow for cases when the same
> function name exists in a single object.
> 	/sys/kernel/livepatch/<patch>/<object>/<function,number>
> 
> The number corresponds to the nth occurrence of the symbol name in
> kallsyms for the patched object.
> 
> An example of patching multiple symbols can be found here:
> 	https://github.com/dynup/kpatch/issues/493

Instead of 'function,number' here and everywhere else, maybe
'function,sympos' would be a little clearer.

Also, s/old_name/function/ in the patch subject to be consistent.

> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  6 +++++-
>  kernel/livepatch/core.c                          | 10 ++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index 5bf42a8..21b6bc1 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -33,7 +33,7 @@ Description:
>  		The object directory contains subdirectories for each function
>  		that is patched within the object.
>  
> -What:		/sys/kernel/livepatch/<patch>/<object>/<function>
> +What:		/sys/kernel/livepatch/<patch>/<object>/<function,number>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
>  Contact:	live-patching@vger.kernel.org
> @@ -41,4 +41,8 @@ Description:
>  		The function directory contains attributes regarding the
>  		properties and state of the patched function.
>  
> +		The directory name contains the patched function name and a
> +		number corresponding to the nth occurrence of the symbol name
> +		in kallsyms for the patched object.
> +
>  		There are currently no such attributes.
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 4eb8691..ed2cbbf 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/<object>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>
> + * /sys/kernel/livepatch/<patch>/<object>/<func,number>
>   */
>  
>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -687,8 +687,14 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	INIT_LIST_HEAD(&func->stack_node);
>  	func->state = KLP_DISABLED;
>  
> +	/* The format for the sysfs directory is <func,number> where number is
> +	 * the occurrence of this symbol in kallsyms. If the user selects 0 for

... of this symbol in kallsyms *for the patched object*.

> +	 * old_sympos, then 1 will be used since a unique symbol will be the
> +	 * first occurrence.
> +	 */
>  	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> -				    &obj->kobj, "%s", func->old_name);
> +				    &obj->kobj, "%s,%lu", func->old_name,
> +				    func->old_sympos ? func->old_sympos : 1);
>  }
>  
>  /* parts of the initialization that is done only when the object is loaded */
> -- 
> 1.9.1

-- 
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]

#1267302 — Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-11 17:40 +0100
SubjectRe: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc
Message-ID<qtGBc-5cy-15@gated-at.bofh.it>
In reply to#1261842
On 11/11/2015 10:29 AM, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default old_sympos will be 0, and patching will only succeed if

Minor typo. old_sympos, should just be sympos.
--chris

> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol will be used for patching if it is valid.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  include/linux/livepatch.h |  5 ++--
>  kernel/livepatch/core.c   | 74 ++++++++++-------------------------------------
>  2 files changed, 18 insertions(+), 61 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index df7b752..fb968a2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -68,8 +68,8 @@ 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)
> + * @val:	address of the referenced symbol
> + * @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
> @@ -78,6 +78,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 26f9778..4eb8691 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -207,45 +207,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;
> -}
> -
>  static int klp_find_verify_func_addr(struct klp_object *obj,
>  				     struct klp_func *func)
>  {
> @@ -261,7 +222,7 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
>   * object is either vmlinux or the kmod being patched).
>   */
>  static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +				    unsigned long *addr, unsigned long sympos)
>  {
>  	const struct kernel_symbol *sym;
>  
> @@ -276,7 +237,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, 0);
> +	return klp_find_object_symbol(pmod->name, name, addr, sympos);
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> @@ -292,24 +253,19 @@ 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,
> -							     &reloc->val, 0);
> -			if (ret)
> -				return ret;
> -		}
> +		/* reloc->val needs to be discovered */
> +		if (reloc->external)
> +			ret = klp_find_external_symbol(pmod,
> +						       reloc->name,
> +						       &reloc->val,
> +						       reloc->sympos);
> +		else
> +			ret = klp_find_object_symbol(obj->mod->name,
> +						     reloc->name,
> +						     &reloc->val,
> +						     reloc->sympos);
> +		if (ret)
> +			return ret;
>  		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>  					     reloc->val + reloc->addend);
>  		if (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]

#1267304 — [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc

FromChris J Arges <chris.j.arges@canonical.com>
Date2015-11-11 17:40 +0100
Subject[PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc
Message-ID<qtGBc-5cy-17@gated-at.bofh.it>
In reply to#1261842
In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. 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 will be used for patching if it is valid.

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index df7b752..fb968a2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -68,8 +68,8 @@ 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)
+ * @val:	address of the referenced symbol
+ * @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
@@ -78,6 +78,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 26f9778..4eb8691 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -207,45 +207,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;
-}
-
 static int klp_find_verify_func_addr(struct klp_object *obj,
 				     struct klp_func *func)
 {
@@ -261,7 +222,7 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
  * object is either vmlinux or the kmod being patched).
  */
 static int klp_find_external_symbol(struct module *pmod, const char *name,
-				    unsigned long *addr)
+				    unsigned long *addr, unsigned long sympos)
 {
 	const struct kernel_symbol *sym;
 
@@ -276,7 +237,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, 0);
+	return klp_find_object_symbol(pmod->name, name, addr, sympos);
 }
 
 static int klp_write_object_relocations(struct module *pmod,
@@ -292,24 +253,19 @@ 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,
-							     &reloc->val, 0);
-			if (ret)
-				return ret;
-		}
+		/* reloc->val needs to be discovered */
+		if (reloc->external)
+			ret = klp_find_external_symbol(pmod,
+						       reloc->name,
+						       &reloc->val,
+						       reloc->sympos);
+		else
+			ret = klp_find_object_symbol(obj->mod->name,
+						     reloc->name,
+						     &reloc->val,
+						     reloc->sympos);
+		if (ret)
+			return ret;
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
 					     reloc->val + reloc->addend);
 		if (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] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web