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

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

[Chris J Arges]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

Re: livepatch: old_name.number scheme in livepatch sysfs directory

[Jessica Yu]

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

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

[Josh Poimboeuf]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

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

[Chris J Arges]

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



 

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

[Josh Poimboeuf]

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

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

[Chris J Arges]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

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

[Miroslav Benes]

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

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

[Miroslav Benes]

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

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

[Petr Mladek]

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

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

[Josh Poimboeuf]

On Tue, Nov 03, 2015 at 11:52:08AM +0100, 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 should be fixed as well.

Even worse, klp_init_func() can be called even if the object hasn't been
loaded.  In that case there's no way to know what the value of n is, and
therefore no way to reliably create the sysfs entry.

Should we create "func,n" in klp_init_object_loaded() instead of
klp_init_func()?

-- 
Josh

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

[Josh Poimboeuf]

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

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

[Josh Poimboeuf]

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

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

[Miroslav Benes]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> On Tue, Nov 03, 2015 at 11:52:08AM +0100, 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 should be fixed as well.
> 
> Even worse, klp_init_func() can be called even if the object hasn't been
> loaded.  In that case there's no way to know what the value of n is, and
> therefore no way to reliably create the sysfs entry.

Ah, right.

> Should we create "func,n" in klp_init_object_loaded() instead of
> klp_init_func()?

So that the function entries in sysfs would be created only when the 
object is loaded? Well, why not, but in that case it could easily confuse 
the user. Object entry would be empty for not loaded object. I would not 
dare to propose to remove such object entries. It would make things worse. 
So maybe we could introduce an attribute in sysfs object entry which would 
say if the object is loaded or not. Or something like that. Hm, we can 
easily mess this up :)

Miroslav

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

[Josh Poimboeuf]

On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Tue, Nov 03, 2015 at 11:52:08AM +0100, 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 should be fixed as well.
> > 
> > Even worse, klp_init_func() can be called even if the object hasn't been
> > loaded.  In that case there's no way to know what the value of n is, and
> > therefore no way to reliably create the sysfs entry.
> 
> Ah, right.
> 
> > Should we create "func,n" in klp_init_object_loaded() instead of
> > klp_init_func()?
> 
> So that the function entries in sysfs would be created only when the 
> object is loaded? Well, why not, but in that case it could easily confuse 
> the user.

Maybe, but I think it would be fine if we document it.  It should only
be relied on by tools, anyway.

> Object entry would be empty for not loaded object. I would not 
> dare to propose to remove such object entries. It would make things worse. 

Why would removing an empty object entry make things worse?

> So maybe we could introduce an attribute in sysfs object entry which would 
> say if the object is loaded or not. Or something like that.

Hm, I'm not sure I see how this would help.

> Hm, we can easily mess this up :)

Agreed 100% :-)

-- 
Josh

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

[Jiri Kosina]

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

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

[Josh Poimboeuf]

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

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

[Chris J Arges]

On 11/03/2015 10:50 AM, Josh Poimboeuf wrote:
> On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
>> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
>> 
>>> On Tue, Nov 03, 2015 at 11:52:08AM +0100, 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 should be fixed as well.
>>> 
>>> Even worse, klp_init_func() can be called even if the object
>>> hasn't been loaded.  In that case there's no way to know what the
>>> value of n is, and therefore no way to reliably create the sysfs
>>> entry.
>> 
>> Ah, right.
>> 
>>> Should we create "func,n" in klp_init_object_loaded() instead of 
>>> klp_init_func()?
>> 
>> So that the function entries in sysfs would be created only when
>> the object is loaded? Well, why not, but in that case it could
>> easily confuse the user.
> 
> Maybe, but I think it would be fine if we document it.  It should
> only be relied on by tools, anyway.
> 
>> Object entry would be empty for not loaded object. I would not dare
>> to propose to remove such object entries. It would make things
>> worse.
> 
> Why would removing an empty object entry make things worse?
> 
>> So maybe we could introduce an attribute in sysfs object entry
>> which would say if the object is loaded or not. Or something like
>> that.
> 
> Hm, I'm not sure I see how this would help.
> 
>> Hm, we can easily mess this up :)
> 
> Agreed 100% :-)
> 

Working on v3 with these suggestions.
- Documentation fixes
- create func,n in klp_init_object_loaded

I'll test unique and non-unique functions being patched. In addition
I'll test this when the object is vmlinux or a module (to test the
object being loaded later).

--chris

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

[Miroslav Benes]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
> > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > 
> > > On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
> > > > 
> > > > 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.
> > > 
> > > Even worse, klp_init_func() can be called even if the object hasn't been
> > > loaded.  In that case there's no way to know what the value of n is, and
> > > therefore no way to reliably create the sysfs entry.
> > 
> > Ah, right.
> > 
> > > Should we create "func,n" in klp_init_object_loaded() instead of
> > > klp_init_func()?
> > 
> > So that the function entries in sysfs would be created only when the 
> > object is loaded? Well, why not, but in that case it could easily confuse 
> > the user.
> 
> Maybe, but I think it would be fine if we document it.  It should only
> be relied on by tools, anyway.

Agreed.

> > Object entry would be empty for not loaded object. I would not 
> > dare to propose to remove such object entries. It would make things worse. 
> 
> Why would removing an empty object entry make things worse?

I think it all comes down to a question whether the sysfs entries say what 
a patch is capable to patch or what this patch is currently patching in 
the system. I am inclined to the former so the removal would make me 
nervous. But I am not against the second approach. We are still in testing 
mode as far as sysfs is concerned so we can try even harsh changes and see 
how it's gonna go.

> > So maybe we could introduce an attribute in sysfs object entry which would 
> > say if the object is loaded or not. Or something like that.
> 
> Hm, I'm not sure I see how this would help.

Hopefully I cleared this up with the above.

Miroslav

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

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > Object entry would be empty for not loaded object. I would not 
> > > dare to propose to remove such object entries. It would make things worse. 
> > 
> > Why would removing an empty object entry make things worse?
> 
> I think it all comes down to a question whether the sysfs entries say what 
> a patch is capable to patch or what this patch is currently patching in 
> the system. I am inclined to the former so the removal would make me 
> nervous. But I am not against the second approach. We are still in testing 
> mode as far as sysfs is concerned so we can try even harsh changes and see 
> how it's gonna go.

I see your point.  This approach only describes what is patched now, but
it doesn't describe what *will* be patched.  Ideally we could find a way
to describe both.

Speaking of harsh changes, here's an idea.

What if we require the patch author to supply the value of 'n' instead
of supplying the symbol address?  We could get rid of 'old_addr' as an
input in klp_func and and replace it with 'old_sympos' which has the
value of 'n'.  Or alternatively we could require old_name to be of the
format "func,n".

That would uniquely identify each patched function, even _before_ the
object is loaded.

It would also fix another big problem we have today, where there's no
way to disambiguate duplicate symbols in modules, for both function
addresses and for relocs.

It would simplify the code in other places as well: no special handling
for kASLR, no need for klp_verify_vmlinux_symbol() vs
klp_find_object_symbol().

A drawback is that it requires the patch author to do a little more due
diligence when filling out klp_func.  But we already require them to be
careful.

Thoughts?

-- 
Josh

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

[Chris J Arges]

On 11/04/2015 10:03 AM, Josh Poimboeuf wrote:
> On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
>> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
>>>> Object entry would be empty for not loaded object. I would not 
>>>> dare to propose to remove such object entries. It would make things worse. 
>>>
>>> Why would removing an empty object entry make things worse?
>>
>> I think it all comes down to a question whether the sysfs entries say what 
>> a patch is capable to patch or what this patch is currently patching in 
>> the system. I am inclined to the former so the removal would make me 
>> nervous. But I am not against the second approach. We are still in testing 
>> mode as far as sysfs is concerned so we can try even harsh changes and see 
>> how it's gonna go.
> 
> I see your point.  This approach only describes what is patched now, but
> it doesn't describe what *will* be patched.  Ideally we could find a way
> to describe both.
> 
> Speaking of harsh changes, here's an idea.
> 
> What if we require the patch author to supply the value of 'n' instead
> of supplying the symbol address?  We could get rid of 'old_addr' as an
> input in klp_func and and replace it with 'old_sympos' which has the
> value of 'n'.  Or alternatively we could require old_name to be of the
> format "func,n".

I like the idea of old_sympos better than modifying the string.
In addition if no old_sympos is specified then it should default to 0,
since this will probably be the more common case.

> 
> That would uniquely identify each patched function, even _before_ the
> object is loaded.
> 
> It would also fix another big problem we have today, where there's no
> way to disambiguate duplicate symbols in modules, for both function
> addresses and for relocs.
> 
> It would simplify the code in other places as well: no special handling
> for kASLR, no need for klp_verify_vmlinux_symbol() vs
> klp_find_object_symbol().
> 
> A drawback is that it requires the patch author to do a little more due
> diligence when filling out klp_func.  But we already require them to be
> careful.
> 
> Thoughts?
> 

I'll hold off on my v3 for now. Very interesting discussion : ).
--chris

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

[Miroslav Benes]

On Wed, 4 Nov 2015, Josh Poimboeuf wrote:

> On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > > Object entry would be empty for not loaded object. I would not 
> > > > dare to propose to remove such object entries. It would make things worse. 
> > > 
> > > Why would removing an empty object entry make things worse?
> > 
> > I think it all comes down to a question whether the sysfs entries say what 
> > a patch is capable to patch or what this patch is currently patching in 
> > the system. I am inclined to the former so the removal would make me 
> > nervous. But I am not against the second approach. We are still in testing 
> > mode as far as sysfs is concerned so we can try even harsh changes and see 
> > how it's gonna go.
> 
> I see your point.  This approach only describes what is patched now, but
> it doesn't describe what *will* be patched.  Ideally we could find a way
> to describe both.
> 
> Speaking of harsh changes, here's an idea.

Which is the very same you proposed last year when I tried to persuade you 
to get rid off old_addr and stuff. I called it crazy I remember :D. So 
here we are again...

> What if we require the patch author to supply the value of 'n' instead
> of supplying the symbol address?  We could get rid of 'old_addr' as an
> input in klp_func and and replace it with 'old_sympos' which has the
> value of 'n'.  Or alternatively we could require old_name to be of the
> format "func,n".
> 
> That would uniquely identify each patched function, even _before_ the
> object is loaded.

I find it reasonable and we should try it. I think that old_sympos should 
have this semantics

0 - default, preserve more or less current behaviour. If the symbol is 
    unique there is no problem. If it is not the patching would fail.
1, 2, ... - occurrence of the symbol in kallsyms.

The advantage is that if the user does not care and is certain that the 
symbol is unique he doesn't have to do anything. If the symbol is not 
unique he still has means how to solve it.

Does it make sense?

> It would also fix another big problem we have today, where there's no
> way to disambiguate duplicate symbols in modules, for both function
> addresses and for relocs.

True.

> It would simplify the code in other places as well: no special handling
> for kASLR, no need for klp_verify_vmlinux_symbol() vs
> klp_find_object_symbol().

Which would be great.

> A drawback is that it requires the patch author to do a little more due
> diligence when filling out klp_func.  But we already require them to be
> careful.

Yes, I don't think this should be a problem.

> Thoughts?

Yup, we should try it. I suppose that the order of the symbols in kallsyms 
table is stable for once-built kernel. It is the order of the symbols in 
the object files, isn't it? And since each livepatch module is built 
against the specific kernel there should be no issues with this.

Regards,
Miroslav

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

[Josh Poimboeuf]

On Thu, Nov 05, 2015 at 04:18:12PM +0100, Miroslav Benes wrote:
> On Wed, 4 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > > > Object entry would be empty for not loaded object. I would not 
> > > > > dare to propose to remove such object entries. It would make things worse. 
> > > > 
> > > > Why would removing an empty object entry make things worse?
> > > 
> > > I think it all comes down to a question whether the sysfs entries say what 
> > > a patch is capable to patch or what this patch is currently patching in 
> > > the system. I am inclined to the former so the removal would make me 
> > > nervous. But I am not against the second approach. We are still in testing 
> > > mode as far as sysfs is concerned so we can try even harsh changes and see 
> > > how it's gonna go.
> > 
> > I see your point.  This approach only describes what is patched now, but
> > it doesn't describe what *will* be patched.  Ideally we could find a way
> > to describe both.
> > 
> > Speaking of harsh changes, here's an idea.
> 
> Which is the very same you proposed last year when I tried to persuade you 
> to get rid off old_addr and stuff. I called it crazy I remember :D. So 
> here we are again...

Ah, I knew I had entertained the idea before, but I forgot we discussed
it.  It is indeed a little crazy.  But this is live patching after all
;-)

> > What if we require the patch author to supply the value of 'n' instead
> > of supplying the symbol address?  We could get rid of 'old_addr' as an
> > input in klp_func and and replace it with 'old_sympos' which has the
> > value of 'n'.  Or alternatively we could require old_name to be of the
> > format "func,n".
> > 
> > That would uniquely identify each patched function, even _before_ the
> > object is loaded.
> 
> I find it reasonable and we should try it. I think that old_sympos should 
> have this semantics
> 
> 0 - default, preserve more or less current behaviour. If the symbol is 
>     unique there is no problem. If it is not the patching would fail.
> 1, 2, ... - occurrence of the symbol in kallsyms.
> 
> The advantage is that if the user does not care and is certain that the 
> symbol is unique he doesn't have to do anything. If the symbol is not 
> unique he still has means how to solve it.
> 
> Does it make sense?

Sounds good!

> > It would also fix another big problem we have today, where there's no
> > way to disambiguate duplicate symbols in modules, for both function
> > addresses and for relocs.
> 
> True.
> 
> > It would simplify the code in other places as well: no special handling
> > for kASLR, no need for klp_verify_vmlinux_symbol() vs
> > klp_find_object_symbol().
> 
> Which would be great.
> 
> > A drawback is that it requires the patch author to do a little more due
> > diligence when filling out klp_func.  But we already require them to be
> > careful.
> 
> Yes, I don't think this should be a problem.
> 
> > Thoughts?
> 
> Yup, we should try it. I suppose that the order of the symbols in kallsyms 
> table is stable for once-built kernel. It is the order of the symbols in 
> the object files, isn't it? And since each livepatch module is built 
> against the specific kernel there should be no issues with this.

The order of the symbols in an object's symbol table does appear to be
the same as the order in kallsyms (per-object).  So yeah, let's try it.

-- 
Josh

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

[Chris J Arges]

On Thu, Nov 05, 2015 at 09:56:56AM -0600, Josh Poimboeuf wrote:
> On Thu, Nov 05, 2015 at 04:18:12PM +0100, Miroslav Benes wrote:
> > On Wed, 4 Nov 2015, Josh Poimboeuf wrote:
> > 
> > > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> > > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > > > > Object entry would be empty for not loaded object. I would not 
> > > > > > dare to propose to remove such object entries. It would make things worse. 
> > > > > 
> > > > > Why would removing an empty object entry make things worse?
> > > > 
> > > > I think it all comes down to a question whether the sysfs entries say what 
> > > > a patch is capable to patch or what this patch is currently patching in 
> > > > the system. I am inclined to the former so the removal would make me 
> > > > nervous. But I am not against the second approach. We are still in testing 
> > > > mode as far as sysfs is concerned so we can try even harsh changes and see 
> > > > how it's gonna go.
> > > 
> > > I see your point.  This approach only describes what is patched now, but
> > > it doesn't describe what *will* be patched.  Ideally we could find a way
> > > to describe both.
> > > 
> > > Speaking of harsh changes, here's an idea.
> > 
> > Which is the very same you proposed last year when I tried to persuade you 
> > to get rid off old_addr and stuff. I called it crazy I remember :D. So 
> > here we are again...
> 
> Ah, I knew I had entertained the idea before, but I forgot we discussed
> it.  It is indeed a little crazy.  But this is live patching after all
> ;-)
> 
> > > What if we require the patch author to supply the value of 'n' instead
> > > of supplying the symbol address?  We could get rid of 'old_addr' as an
> > > input in klp_func and and replace it with 'old_sympos' which has the
> > > value of 'n'.  Or alternatively we could require old_name to be of the
> > > format "func,n".
> > > 
> > > That would uniquely identify each patched function, even _before_ the
> > > object is loaded.
> > 
> > I find it reasonable and we should try it. I think that old_sympos should 
> > have this semantics
> > 
> > 0 - default, preserve more or less current behaviour. If the symbol is 
> >     unique there is no problem. If it is not the patching would fail.
> > 1, 2, ... - occurrence of the symbol in kallsyms.
> > 
> > The advantage is that if the user does not care and is certain that the 
> > symbol is unique he doesn't have to do anything. If the symbol is not 
> > unique he still has means how to solve it.
> > 
> > Does it make sense?
> 
> Sounds good!
> 
> > > It would also fix another big problem we have today, where there's no
> > > way to disambiguate duplicate symbols in modules, for both function
> > > addresses and for relocs.
> > 
> > True.
> > 
> > > It would simplify the code in other places as well: no special handling
> > > for kASLR, no need for klp_verify_vmlinux_symbol() vs
> > > klp_find_object_symbol().
> > 
> > Which would be great.
> > 
> > > A drawback is that it requires the patch author to do a little more due
> > > diligence when filling out klp_func.  But we already require them to be
> > > careful.
> > 
> > Yes, I don't think this should be a problem.
> > 
> > > Thoughts?
> > 
> > Yup, we should try it. I suppose that the order of the symbols in kallsyms 
> > table is stable for once-built kernel. It is the order of the symbols in 
> > the object files, isn't it? And since each livepatch module is built 
> > against the specific kernel there should be no issues with this.
> 
> The order of the symbols in an object's symbol table does appear to be
> the same as the order in kallsyms (per-object).  So yeah, let's try it.
> 
> -- 
> Josh
>

Great! Using this feedback to create the next patch. I'll post something in the
next few days.

--chris
 

[PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Chris J Arges]

In cases of duplicate symbols in vmlinux, old_sympos will be used to
disambiguate instead of old_addr. Normally old_sympos will be 0, and
default to only returning the first found instance of that symbol. If an
incorrect symbol position is specified then livepatching will fail.
Finally, old_addr is now an internal structure element and not to be
specified by the user.

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  6 ++-
 include/linux/livepatch.h                        | 20 ++++---
 kernel/livepatch/core.c                          | 66 ++++++++++++++++--------
 3 files changed, 61 insertions(+), 31 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..986e06d 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
+ * @old_sympos: a hint indicating which symbol position the old function
  *		can be found (optional, vmlinux patches only)
+ * @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,20 @@ 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 the vmlinux object.  If this information is not
+	 * present, the first symbol located with kallsyms is used. This value
+	 * corresponds to the nth occurrence of the symbol name in kallsyms for
+	 * the patched object. If the name is not unique and old_sympos is not
+	 * provided, the patch application fails as there is no way to resolve
+	 * the ambiguity.
 	 */
-	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..1dd0d44 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,
@@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
 	args->addr = addr;
 	args->count++;
 
+	/*
+	 * ensure count matches the symbol position
+	 */
+	if (args->pos == (args->count-1))
+		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, then check that 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)
-		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
-		       args.count, name, objname);
+	else if (sympos != (args.count - 1))
+		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,20 +251,19 @@ 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 sympos = 0;
 	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_sympos && !klp_is_module(obj))
+		sympos = func->old_sympos;
 
-	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);
+	/*
+	 * Verify the symbol, find old_addr, and write it to the structure.
+	 * By default sympos will be 0 and thus will only look for the first
+	 * occurrence. If another value is specified then that will be used.
+	 */
+	ret = klp_find_object_symbol(obj->name, func->old_name,
+				     &func->old_addr, sympos);
 
 	return ret;
 }
@@ -277,7 +288,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 +318,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;
 		}
@@ -587,7 +598,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,
@@ -732,8 +743,7 @@ 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);
+	return 0;
 }
 
 /* parts of the initialization that is done only when the object is loaded */
@@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 			return ret;
 	}
 
+	/*
+	 * for each function initialize and add, old_sympos will be already
+	 * verified at this point
+	 */
+	klp_for_each_func(obj, func) {
+		ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
+				    &obj->kobj, "%s,%lu", func->old_name,
+				    func->old_sympos ? func->old_sympos : 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Josh Poimboeuf]

I'd recommend splitting this up into two separate patches:

1. introduce old_sympos
2. change the sysfs interface

On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> default to only returning the first found instance of that symbol. If an
> incorrect symbol position is specified then livepatching will fail.

In the case of old_sympos == 0, instead of just returning the first
symbol it finds, I think it should ensure that the symbol is unique.  As
Miroslav suggested:

  0 - default, preserve more or less current behaviour. If the symbol is 
      unique there is no problem. If it is not the patching would fail.
  1, 2, ... - occurrence of the symbol in kallsyms.
  
  The advantage is that if the user does not care and is certain that the 
  symbol is unique he doesn't have to do anything. If the symbol is not 
  unique he still has means how to solve it.

> Finally, old_addr is now an internal structure element and not to be
> specified by the user.
> 
> 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>

Period should be changed to a comma.

> 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 ++-
>  include/linux/livepatch.h                        | 20 ++++---
>  kernel/livepatch/core.c                          | 66 ++++++++++++++++--------
>  3 files changed, 61 insertions(+), 31 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/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..986e06d 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
> + * @old_sympos: a hint indicating which symbol position the old function
>   *		can be found (optional, vmlinux patches only)

Why is old_sympos only checked for vmlinux patches only?  It's a
per-object count, so it should work for modules as well.

> + * @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,20 @@ 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 the vmlinux object.  If this information is not
> +	 * present, the first symbol located with kallsyms is used. This value
> +	 * corresponds to the nth occurrence of the symbol name in kallsyms for
> +	 * the patched object. If the name is not unique and old_sympos is not
> +	 * provided, the patch application fails as there is no way to resolve
> +	 * the ambiguity.
>  	 */
> -	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..1dd0d44 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,
> @@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
>  	args->addr = addr;
>  	args->count++;
>  
> +	/*
> +	 * ensure count matches the symbol position
> +	 */
> +	if (args->pos == (args->count-1))
> +		return 1;
> +

The code can be simplified a bit if args->addr only gets set when
args->pos is a match.  Then klp_find_object_symbol() only needs to check
args.addr to see if a match was found.

>  	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, then check that 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)
> -		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
> -		       args.count, name, objname);
> +	else if (sympos != (args.count - 1))
> +		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,20 +251,19 @@ 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 sympos = 0;
>  	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_sympos && !klp_is_module(obj))
> +		sympos = func->old_sympos;
>  
> -	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);
> +	/*
> +	 * Verify the symbol, find old_addr, and write it to the structure.
> +	 * By default sympos will be 0 and thus will only look for the first
> +	 * occurrence. If another value is specified then that will be used.
> +	 */
> +	ret = klp_find_object_symbol(obj->name, func->old_name,
> +				     &func->old_addr, sympos);
>  
>  	return ret;
>  }
> @@ -277,7 +288,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 +318,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);

I think it would be a good idea to also add old_sympos to klp_reloc so
the relocation code is consistent with the klp_func symbol addressing.

>  			if (ret)
>  				return ret;
>  		}
> @@ -587,7 +598,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,
> @@ -732,8 +743,7 @@ 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);
> +	return 0;
>  }

The sysfs entry can still be created here, since the function name and
sympos are both already known.

>  
>  /* parts of the initialization that is done only when the object is loaded */
> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  			return ret;
>  	}
>  
> +	/*
> +	 * for each function initialize and add, old_sympos will be already
> +	 * verified at this point
> +	 */
> +	klp_for_each_func(obj, func) {
> +		ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> +				    &obj->kobj, "%s,%lu", func->old_name,
> +				    func->old_sympos ? func->old_sympos : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

-- 
Josh

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Chris J Arges]

On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> I'd recommend splitting this up into two separate patches:
> 
> 1. introduce old_sympos
> 2. change the sysfs interface
> 
> On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
>> In cases of duplicate symbols in vmlinux, old_sympos will be used to
>> disambiguate instead of old_addr. Normally old_sympos will be 0, and
>> default to only returning the first found instance of that symbol. If an
>> incorrect symbol position is specified then livepatching will fail.
> 
> In the case of old_sympos == 0, instead of just returning the first
> symbol it finds, I think it should ensure that the symbol is unique.  As
> Miroslav suggested:
> 
>   0 - default, preserve more or less current behaviour. If the symbol is 
>       unique there is no problem. If it is not the patching would fail.
>   1, 2, ... - occurrence of the symbol in kallsyms.
>   
>   The advantage is that if the user does not care and is certain that the 
>   symbol is unique he doesn't have to do anything. If the symbol is not 
>   unique he still has means how to solve it.
> 

So one part that will be confusing here is as follows.

If '0' is specified for old_sympos, should the symbol be 'func_name,0'
or 'func_name,1' provided we have a unique symbol? We could also default
to 'what the user provides', but this seems odd.

Another option would be to use no postfix when 0 is given, and only
introduce the ',n' postfix if old_sympos is > 0.

--chris

>> Finally, old_addr is now an internal structure element and not to be
>> specified by the user.
>>
>> 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>
> 
> Period should be changed to a comma.
> 
>> 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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>>  Documentation/ABI/testing/sysfs-kernel-livepatch |  6 ++-
>>  include/linux/livepatch.h                        | 20 ++++---
>>  kernel/livepatch/core.c                          | 66 ++++++++++++++++--------
>>  3 files changed, 61 insertions(+), 31 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..986e06d 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
>> + * @old_sympos: a hint indicating which symbol position the old function
>>   *		can be found (optional, vmlinux patches only)
> 
> Why is old_sympos only checked for vmlinux patches only?  It's a
> per-object count, so it should work for modules as well.
> 
>> + * @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,20 @@ 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 the vmlinux object.  If this information is not
>> +	 * present, the first symbol located with kallsyms is used. This value
>> +	 * corresponds to the nth occurrence of the symbol name in kallsyms for
>> +	 * the patched object. If the name is not unique and old_sympos is not
>> +	 * provided, the patch application fails as there is no way to resolve
>> +	 * the ambiguity.
>>  	 */
>> -	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..1dd0d44 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,
>> @@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
>>  	args->addr = addr;
>>  	args->count++;
>>  
>> +	/*
>> +	 * ensure count matches the symbol position
>> +	 */
>> +	if (args->pos == (args->count-1))
>> +		return 1;
>> +
> 
> The code can be simplified a bit if args->addr only gets set when
> args->pos is a match.  Then klp_find_object_symbol() only needs to check
> args.addr to see if a match was found.
> 
>>  	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, then check that 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)
>> -		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
>> -		       args.count, name, objname);
>> +	else if (sympos != (args.count - 1))
>> +		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,20 +251,19 @@ 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 sympos = 0;
>>  	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_sympos && !klp_is_module(obj))
>> +		sympos = func->old_sympos;
>>  
>> -	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);
>> +	/*
>> +	 * Verify the symbol, find old_addr, and write it to the structure.
>> +	 * By default sympos will be 0 and thus will only look for the first
>> +	 * occurrence. If another value is specified then that will be used.
>> +	 */
>> +	ret = klp_find_object_symbol(obj->name, func->old_name,
>> +				     &func->old_addr, sympos);
>>  
>>  	return ret;
>>  }
>> @@ -277,7 +288,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 +318,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);
> 
> I think it would be a good idea to also add old_sympos to klp_reloc so
> the relocation code is consistent with the klp_func symbol addressing.
>

So you are thinking as an optional external field as well? I'll have to
look at this a bit more but makes sense to me.
--chris


>>  			if (ret)
>>  				return ret;
>>  		}
>> @@ -587,7 +598,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,
>> @@ -732,8 +743,7 @@ 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);
>> +	return 0;
>>  }
> 
> The sysfs entry can still be created here, since the function name and
> sympos are both already known.
> 
>>  
>>  /* parts of the initialization that is done only when the object is loaded */
>> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>>  			return ret;
>>  	}
>>  
>> +	/*
>> +	 * for each function initialize and add, old_sympos will be already
>> +	 * verified at this point
>> +	 */
>> +	klp_for_each_func(obj, func) {
>> +		ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
>> +				    &obj->kobj, "%s,%lu", func->old_name,
>> +				    func->old_sympos ? func->old_sympos : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Josh Poimboeuf]

On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:
> On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> > I'd recommend splitting this up into two separate patches:
> > 
> > 1. introduce old_sympos
> > 2. change the sysfs interface
> > 
> > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> >> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> >> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> >> default to only returning the first found instance of that symbol. If an
> >> incorrect symbol position is specified then livepatching will fail.
> > 
> > In the case of old_sympos == 0, instead of just returning the first
> > symbol it finds, I think it should ensure that the symbol is unique.  As
> > Miroslav suggested:
> > 
> >   0 - default, preserve more or less current behaviour. If the symbol is 
> >       unique there is no problem. If it is not the patching would fail.
> >   1, 2, ... - occurrence of the symbol in kallsyms.
> >   
> >   The advantage is that if the user does not care and is certain that the 
> >   symbol is unique he doesn't have to do anything. If the symbol is not 
> >   unique he still has means how to solve it.
> > 
> 
> So one part that will be confusing here is as follows.
> 
> If '0' is specified for old_sympos, should the symbol be 'func_name,0'
> or 'func_name,1' provided we have a unique symbol? We could also default
> to 'what the user provides', but this seems odd.

I don't feel strongly either way, but I think using the same number the
user provides is fine, since it makes the sysfs interface consistent
with the old_sympos usage.

> Another option would be to use no postfix when 0 is given, and only
> introduce the ',n' postfix if old_sympos is > 0.

IMO always having a suffix is good, as it makes parsing less surprising
and less error-prone.

> >>  static int klp_write_object_relocations(struct module *pmod,
> >> @@ -307,7 +318,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);
> > 
> > I think it would be a good idea to also add old_sympos to klp_reloc so
> > the relocation code is consistent with the klp_func symbol addressing.
> >
> 
> So you are thinking as an optional external field as well? I'll have to
> look at this a bit more but makes sense to me.

Yeah, the semantics would be the same as klp_func.old_sympos.  We could
add a new klp_reloc.sympos and make klp_reloc.val a private field.

-- 
Josh

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Miroslav Benes]

On Mon, 9 Nov 2015, Josh Poimboeuf wrote:

> On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:
> > On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> > > I'd recommend splitting this up into two separate patches:
> > > 
> > > 1. introduce old_sympos
> > > 2. change the sysfs interface
> > > 
> > > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> > >> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> > >> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> > >> default to only returning the first found instance of that symbol. If an
> > >> incorrect symbol position is specified then livepatching will fail.
> > > 
> > > In the case of old_sympos == 0, instead of just returning the first
> > > symbol it finds, I think it should ensure that the symbol is unique.  As
> > > Miroslav suggested:
> > > 
> > >   0 - default, preserve more or less current behaviour. If the symbol is 
> > >       unique there is no problem. If it is not the patching would fail.
> > >   1, 2, ... - occurrence of the symbol in kallsyms.
> > >   
> > >   The advantage is that if the user does not care and is certain that the 
> > >   symbol is unique he doesn't have to do anything. If the symbol is not 
> > >   unique he still has means how to solve it.
> > > 
> > 
> > So one part that will be confusing here is as follows.
> > 
> > If '0' is specified for old_sympos, should the symbol be 'func_name,0'
> > or 'func_name,1' provided we have a unique symbol? We could also default
> > to 'what the user provides', but this seems odd.
> 
> I don't feel strongly either way, but I think using the same number the
> user provides is fine, since it makes the sysfs interface consistent
> with the old_sympos usage.

I think it should be func_name,1 even if '0' is specified and the symbol 
is unique. Because if we say that 1, 2, ... is the occurrence of the 
symbol in kallsyms it should stay that way everywhere. Hence for 
old_sympos == 0 it is func_name,1 in sysfs; for 1 it is still func_name,1; 
for 2 it is func_name,2 and so on.

And I'd add this to sysfs documentation.

> > Another option would be to use no postfix when 0 is given, and only
> > introduce the ',n' postfix if old_sympos is > 0.
> 
> IMO always having a suffix is good, as it makes parsing less surprising
> and less error-prone.

Agreed.

> > >>  static int klp_write_object_relocations(struct module *pmod,
> > >> @@ -307,7 +318,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);
> > > 
> > > I think it would be a good idea to also add old_sympos to klp_reloc so
> > > the relocation code is consistent with the klp_func symbol addressing.
> > >
> > 
> > So you are thinking as an optional external field as well? I'll have to
> > look at this a bit more but makes sense to me.
> 
> Yeah, the semantics would be the same as klp_func.old_sympos.  We could
> add a new klp_reloc.sympos and make klp_reloc.val a private field.

Agreed as well.

Miroslav

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Miroslav Benes]

On Mon, 9 Nov 2015, Chris J Arges wrote:

> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> default to only returning the first found instance of that symbol. If an
> incorrect symbol position is specified then livepatching will fail.
> Finally, old_addr is now an internal structure element and not to be
> specified by the user.

Hi,

Josh has already mentioned it, but in this case '0' would be same as '1'. 
'0' should fail if the symbol is ambiguous.

Few more things follow, but Josh pointed out the issues.

[...]

> @@ -239,20 +251,19 @@ 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 sympos = 0;
>  	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_sympos && !klp_is_module(obj))
> +		sympos = func->old_sympos;

We need to deal with ambiguity in modules as well. 

Also, maybe the function could be renamed, because there would be no 
verification here in the future.

>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -732,8 +743,7 @@ 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);
> +	return 0;
>  }
>  
>  /* parts of the initialization that is done only when the object is loaded */
> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  			return ret;
>  	}
>  
> +	/*
> +	 * for each function initialize and add, old_sympos will be already
> +	 * verified at this point
> +	 */
> +	klp_for_each_func(obj, func) {
> +		ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> +				    &obj->kobj, "%s,%lu", func->old_name,
> +				    func->old_sympos ? func->old_sympos : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

There is a problem with error handling in klp_init_object() after the 
change. 

free:
        klp_free_funcs_limited(obj, func);
        kobject_put(&obj->kobj);
        return ret;

This snippet ensures that all already created sysfs func entries are 
destroyed. 'func' is the function which klp_init_func() failed for (or 
'{}' if nothing failed). When you move kobject_init_and_add() with the 
loop to klp_init_object_loaded(), we do not know where the exact problem 
was in klp_init_object(). So I agree with Josh that it can stay in 
klp_init_func(). old_sympos is defined and if the following 
klp_find_verify_func_addr() fails (for example when old_sympos is '0' and 
the symbol is not unique) we deal with it here in klp_init_object() 
correctly.

Thanks,
Miroslav

Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

[Josh Poimboeuf]

On Tue, Nov 10, 2015 at 09:49:09AM +0100, Miroslav Benes wrote:
> On Mon, 9 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:
> > > On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> > > > I'd recommend splitting this up into two separate patches:
> > > > 
> > > > 1. introduce old_sympos
> > > > 2. change the sysfs interface
> > > > 
> > > > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> > > >> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> > > >> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> > > >> default to only returning the first found instance of that symbol. If an
> > > >> incorrect symbol position is specified then livepatching will fail.
> > > > 
> > > > In the case of old_sympos == 0, instead of just returning the first
> > > > symbol it finds, I think it should ensure that the symbol is unique.  As
> > > > Miroslav suggested:
> > > > 
> > > >   0 - default, preserve more or less current behaviour. If the symbol is 
> > > >       unique there is no problem. If it is not the patching would fail.
> > > >   1, 2, ... - occurrence of the symbol in kallsyms.
> > > >   
> > > >   The advantage is that if the user does not care and is certain that the 
> > > >   symbol is unique he doesn't have to do anything. If the symbol is not 
> > > >   unique he still has means how to solve it.
> > > > 
> > > 
> > > So one part that will be confusing here is as follows.
> > > 
> > > If '0' is specified for old_sympos, should the symbol be 'func_name,0'
> > > or 'func_name,1' provided we have a unique symbol? We could also default
> > > to 'what the user provides', but this seems odd.
> > 
> > I don't feel strongly either way, but I think using the same number the
> > user provides is fine, since it makes the sysfs interface consistent
> > with the old_sympos usage.
> 
> I think it should be func_name,1 even if '0' is specified and the symbol 
> is unique. Because if we say that 1, 2, ... is the occurrence of the 
> symbol in kallsyms it should stay that way everywhere. Hence for 
> old_sympos == 0 it is func_name,1 in sysfs; for 1 it is still func_name,1; 
> for 2 it is func_name,2 and so on.
> 
> And I'd add this to sysfs documentation.

That makes sense, sounds fine to me.

-- 
Josh

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

[Chris J Arges]

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

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

[Josh Poimboeuf]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

[PATCH 4/4 v5] livepatch: function,sympos scheme in livepatch sysfs directory

[Chris J Arges]

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

The sympos 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..da87f43 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,sympos>
 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
+		sympos 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 071895f..4a59402 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -532,7 +532,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>/<function,sympos>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -677,8 +677,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 <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. 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

[PATCH 3/3 v6] livepatch: function,sympos scheme in livepatch sysfs directory

[Chris J Arges]

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

The sympos 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..da87f43 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,sympos>
 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
+		sympos 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 5cfe7b5..53ba0ae 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -534,7 +534,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>/<function,sympos>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -679,8 +679,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 <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. 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

[PATCH 3/3 v7] livepatch: function,sympos scheme in livepatch sysfs directory

[Chris J Arges]

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

The sympos 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..da87f43 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,sympos>
 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
+		sympos 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 9d98c7b..2a1fd1f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -534,7 +534,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>/<function,sympos>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -679,8 +679,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 <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. 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

[PATCH 3/3 v8] livepatch: function,sympos scheme in livepatch sysfs directory

[Chris J Arges]

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

The sympos 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..da87f43 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,sympos>
 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
+		sympos 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 c9f924e..0e86c45 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -534,7 +534,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>/<function,sympos>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -679,8 +679,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 <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. 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

Re: [PATCH 3/3 v8] livepatch: function,sympos scheme in livepatch sysfs directory

[Miroslav Benes]

On Fri, 20 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,sympos>
> 
> The sympos 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>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

[PATCH 3/3 v9] livepatch: function,sympos scheme in livepatch sysfs directory

[Chris J Arges]

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

The sympos 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..da87f43 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,sympos>
 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
+		sympos 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 e842534..94893e8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -535,7 +535,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>/<function,sympos>
  */
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -680,8 +680,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 <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. 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