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

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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: [email protected]
> 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

2015-11-02 20:16:25

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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: [email protected]
> > 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

2015-11-02 20:32:46

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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: [email protected]
> > > 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

2015-11-02 23:00:33

[permalink] [raw]

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

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 <[email protected]>
---
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: [email protected]
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

2015-11-03 09:50:12

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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: [email protected]
> 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

2015-11-03 10:52:12

[permalink] [raw]

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

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

2015-11-03 12:44:48

by Petr Mladek

[permalink] [raw]

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

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

2015-11-03 14:58:49

[permalink] [raw]

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

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

2015-11-03 15:03:10

[permalink] [raw]

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

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

2015-11-03 15:03:50

[permalink] [raw]

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

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

2015-11-03 16:09:55

[permalink] [raw]

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

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

2015-11-03 16:51:04

[permalink] [raw]

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

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

2015-11-03 19:57:26

by Jiri Kosina

[permalink] [raw]

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

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

2015-11-03 20:06:13

[permalink] [raw]

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

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

2015-11-03 20:42:24

[permalink] [raw]

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

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

2015-11-04 09:52:56

[permalink] [raw]

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

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

2015-11-04 16:03:59

[permalink] [raw]

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

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

2015-11-04 16:18:00

[permalink] [raw]

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

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

2015-11-05 15:18:15

[permalink] [raw]

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

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

2015-11-05 15:57:00

[permalink] [raw]

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

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

2015-11-05 16:07:40

[permalink] [raw]

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

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

2015-11-09 16:16:13

[permalink] [raw]

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

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 <[email protected]>
---
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: [email protected]
@@ -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

2015-11-09 20:56:24

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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: [email protected]
> @@ -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

2015-11-09 23:01:13

[permalink] [raw]

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

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 <[email protected]>
>> ---
>> 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: [email protected]
>> @@ -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
>>
>

2015-11-10 04:54:37

[permalink] [raw]

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

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

2015-11-10 08:49:12

[permalink] [raw]

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

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

2015-11-10 09:02:21

[permalink] [raw]

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

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

2015-11-10 13:40:20