For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
same, but the rest is not.
Then function will never be patched, although function name and address
are provided both. The reason caused this bug is livepatch cannt
recognize the function name.
Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
and address, if provided. Once they are matched, we can confirm that the
patched function is found.
Signed-off-by: Minfei Huang <[email protected]>
---
kernel/livepatch/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ff42c29..eed719d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -210,7 +210,7 @@ static int klp_verify_vmlinux_callback(void *data, const char *name,
struct klp_verify_args *args = data;
if (!mod &&
- !strcmp(args->name, name) &&
+ !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
args->addr == addr)
return 1;
@@ -226,7 +226,7 @@ static int klp_verify_module_callback(void *data, const char *name,
return 0;
if (!strcmp(args->objname, mod->name) &&
- !strcmp(args->name, name) &&
+ !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
args->addr == addr)
return 1;
--
2.2.2
On Sun 2015-04-12 21:15:54, Minfei Huang wrote:
> For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> same, but the rest is not.
>
> Then function will never be patched, although function name and address
> are provided both. The reason caused this bug is livepatch cannt
> recognize the function name.
>
> Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> and address, if provided. Once they are matched, we can confirm that the
> patched function is found.
This patch kind of make sense for vmlinux addresses. But are there
functions with such a long names (>128 characters)? I hope not. They
would never fit 80 characters per-line.
In each case, it does not make sense for modules because we are
not able to find the symbol via old_addr.
Best Regards,
Petr
> Signed-off-by: Minfei Huang <[email protected]>
> ---
> kernel/livepatch/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ff42c29..eed719d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -210,7 +210,7 @@ static int klp_verify_vmlinux_callback(void *data, const char *name,
> struct klp_verify_args *args = data;
>
> if (!mod &&
> - !strcmp(args->name, name) &&
> + !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
> args->addr == addr)
> return 1;
>
> @@ -226,7 +226,7 @@ static int klp_verify_module_callback(void *data, const char *name,
> return 0;
>
> if (!strcmp(args->objname, mod->name) &&
> - !strcmp(args->name, name) &&
> + !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
> args->addr == addr)
> return 1;
>
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/13/15 at 10:44P, Petr Mladek wrote:
> On Sun 2015-04-12 21:15:54, Minfei Huang wrote:
> > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > same, but the rest is not.
> >
> > Then function will never be patched, although function name and address
> > are provided both. The reason caused this bug is livepatch cannt
> > recognize the function name.
> >
> > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > and address, if provided. Once they are matched, we can confirm that the
> > patched function is found.
>
> This patch kind of make sense for vmlinux addresses. But are there
> functions with such a long names (>128 characters)? I hope not. They
> would never fit 80 characters per-line.
>
> In each case, it does not make sense for modules because we are
> not able to find the symbol via old_addr.
>
Hi, Petr.
Thanks for reviewing my patches.
Yes, You are right that the function address is changable on different
system.
The purpose to add these patches is to avoid the case that function name
is larger enough to exceed 128.
Meanwhile I think we can not predict the behavior what user want to do.
Thanks
Minfei
> Best Regards,
> Petr
>
> > Signed-off-by: Minfei Huang <[email protected]>
> > ---
> > kernel/livepatch/core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index ff42c29..eed719d 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -210,7 +210,7 @@ static int klp_verify_vmlinux_callback(void *data, const char *name,
> > struct klp_verify_args *args = data;
> >
> > if (!mod &&
> > - !strcmp(args->name, name) &&
> > + !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
> > args->addr == addr)
> > return 1;
> >
> > @@ -226,7 +226,7 @@ static int klp_verify_module_callback(void *data, const char *name,
> > return 0;
> >
> > if (!strcmp(args->objname, mod->name) &&
> > - !strcmp(args->name, name) &&
> > + !strncmp(args->name, name, KSYM_NAME_LEN-1) &&
> > args->addr == addr)
> > return 1;
> >
> > --
> > 2.2.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe live-patching" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> same, but the rest is not.
>
> Then function will never be patched, although function name and address
> are provided both. The reason caused this bug is livepatch cannt
> recognize the function name.
>
> Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> and address, if provided. Once they are matched, we can confirm that the
> patched function is found.
>From scripts/kallsyms.c:
if (strlen(str) > KSYM_NAME_LEN) {
fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
"Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
str, strlen(str), KSYM_NAME_LEN);
return -1;
}
So I think such a long symbol name wouldn't be added to the kallsyms
database in the first place.
--
Josh
On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > same, but the rest is not.
> >
> > Then function will never be patched, although function name and address
> > are provided both. The reason caused this bug is livepatch cannt
> > recognize the function name.
> >
> > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > and address, if provided. Once they are matched, we can confirm that the
> > patched function is found.
>
> From scripts/kallsyms.c:
>
> if (strlen(str) > KSYM_NAME_LEN) {
> fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> str, strlen(str), KSYM_NAME_LEN);
> return -1;
> }
>
> So I think such a long symbol name wouldn't be added to the kallsyms
> database in the first place.
>
Actually, kernel allows overlength function name to be used. Following
is my testing module.
We can got the address in /proc/kallsyms.
$ cat /proc/kallsyms | grep sysfs_print
ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
ffffffffa0000010 t kobj_release [sysfs_print]
ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
ffffffffa00004e0 b root_kobj [sysfs_print]
ffffffffa0000200 d print_ktype [sysfs_print]
ffffffffa00004a0 b print_kobj [sysfs_print]
ffffffffa000004c t sys_print_exit [sysfs_print]
ffffffffa0000144 r __func__.14514 [sysfs_print]
ffffffffa0000230 d kobj_attrs [sysfs_print]
ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
ffffffffa0000260 d __this_module [sysfs_print]
ffffffffa000004c t cleanup_module [sysfs_print]
Code:
static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
const char *buf, size_t count)
{
return count;
}
static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
}
static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
static struct attribute *kobj_attrs[] = {
&sys_print_kobj_attr.attr,
NULL
};
Thanks
Minfei
> --
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > same, but the rest is not.
> > >
> > > Then function will never be patched, although function name and address
> > > are provided both. The reason caused this bug is livepatch cannt
> > > recognize the function name.
> > >
> > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > and address, if provided. Once they are matched, we can confirm that the
> > > patched function is found.
> >
> > From scripts/kallsyms.c:
> >
> > if (strlen(str) > KSYM_NAME_LEN) {
> > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > str, strlen(str), KSYM_NAME_LEN);
> > return -1;
> > }
> >
> > So I think such a long symbol name wouldn't be added to the kallsyms
> > database in the first place.
> >
>
> Actually, kernel allows overlength function name to be used. Following
> is my testing module.
>
> We can got the address in /proc/kallsyms.
> $ cat /proc/kallsyms | grep sysfs_print
> ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> ffffffffa0000010 t kobj_release [sysfs_print]
> ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> ffffffffa00004e0 b root_kobj [sysfs_print]
> ffffffffa0000200 d print_ktype [sysfs_print]
> ffffffffa00004a0 b print_kobj [sysfs_print]
> ffffffffa000004c t sys_print_exit [sysfs_print]
> ffffffffa0000144 r __func__.14514 [sysfs_print]
> ffffffffa0000230 d kobj_attrs [sysfs_print]
> ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> ffffffffa0000260 d __this_module [sysfs_print]
> ffffffffa000004c t cleanup_module [sysfs_print]
>
> Code:
>
> static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> const char *buf, size_t count)
> {
> return count;
> }
>
> static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> }
>
> static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> static struct attribute *kobj_attrs[] = {
> &sys_print_kobj_attr.attr,
> NULL
> };
>
Hm, this seems like a kallsyms bug. IMO it should either fail the build
or omit the symbol from the kallsyms db. Truncating it seems dangerous
and counterintuitive.
But regardless I really don't see a good reason to encourage this kind
of insanity in the livepatch code.
--
Josh
On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > same, but the rest is not.
> > > >
> > > > Then function will never be patched, although function name and address
> > > > are provided both. The reason caused this bug is livepatch cannt
> > > > recognize the function name.
> > > >
> > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > and address, if provided. Once they are matched, we can confirm that the
> > > > patched function is found.
> > >
> > > From scripts/kallsyms.c:
> > >
> > > if (strlen(str) > KSYM_NAME_LEN) {
> > > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > str, strlen(str), KSYM_NAME_LEN);
> > > return -1;
> > > }
> > >
> > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > database in the first place.
> > >
> >
> > Actually, kernel allows overlength function name to be used. Following
> > is my testing module.
> >
> > We can got the address in /proc/kallsyms.
> > $ cat /proc/kallsyms | grep sysfs_print
> > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > ffffffffa0000010 t kobj_release [sysfs_print]
> > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > ffffffffa00004e0 b root_kobj [sysfs_print]
> > ffffffffa0000200 d print_ktype [sysfs_print]
> > ffffffffa00004a0 b print_kobj [sysfs_print]
> > ffffffffa000004c t sys_print_exit [sysfs_print]
> > ffffffffa0000144 r __func__.14514 [sysfs_print]
> > ffffffffa0000230 d kobj_attrs [sysfs_print]
> > ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> > ffffffffa0000260 d __this_module [sysfs_print]
> > ffffffffa000004c t cleanup_module [sysfs_print]
> >
> > Code:
> >
> > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > const char *buf, size_t count)
> > {
> > return count;
> > }
> >
> > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > struct kobj_attribute *attr, char *buf)
> > {
> > return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > }
> >
> > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > static struct attribute *kobj_attrs[] = {
> > &sys_print_kobj_attr.attr,
> > NULL
> > };
> >
>
> Hm, this seems like a kallsyms bug. IMO it should either fail the build
> or omit the symbol from the kallsyms db. Truncating it seems dangerous
> and counterintuitive.
>
Kallsyms will record all of the function name, without truncating it.
But the kallsyms will return the truncated function name which is max to
127.
> But regardless I really don't see a good reason to encourage this kind
> of insanity in the livepatch code.
>
Yes, the above code is terrible, but we cannt stop user composing like
that.
Once the function name is like above, user will never have chance to use
livepatch.
Thanks
Minfei
> --
> Josh
On Tue, Apr 14, 2015 at 01:03:48PM +0800, Minfei Huang wrote:
> On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> > On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > > same, but the rest is not.
> > > > >
> > > > > Then function will never be patched, although function name and address
> > > > > are provided both. The reason caused this bug is livepatch cannt
> > > > > recognize the function name.
> > > > >
> > > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > > and address, if provided. Once they are matched, we can confirm that the
> > > > > patched function is found.
> > > >
> > > > From scripts/kallsyms.c:
> > > >
> > > > if (strlen(str) > KSYM_NAME_LEN) {
> > > > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > > str, strlen(str), KSYM_NAME_LEN);
> > > > return -1;
> > > > }
> > > >
> > > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > > database in the first place.
> > > >
> > >
> > > Actually, kernel allows overlength function name to be used. Following
> > > is my testing module.
> > >
> > > We can got the address in /proc/kallsyms.
> > > $ cat /proc/kallsyms | grep sysfs_print
> > > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > ffffffffa0000010 t kobj_release [sysfs_print]
> > > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > ffffffffa00004e0 b root_kobj [sysfs_print]
> > > ffffffffa0000200 d print_ktype [sysfs_print]
> > > ffffffffa00004a0 b print_kobj [sysfs_print]
> > > ffffffffa000004c t sys_print_exit [sysfs_print]
> > > ffffffffa0000144 r __func__.14514 [sysfs_print]
> > > ffffffffa0000230 d kobj_attrs [sysfs_print]
> > > ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> > > ffffffffa0000260 d __this_module [sysfs_print]
> > > ffffffffa000004c t cleanup_module [sysfs_print]
> > >
> > > Code:
> > >
> > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > > const char *buf, size_t count)
> > > {
> > > return count;
> > > }
> > >
> > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > > struct kobj_attribute *attr, char *buf)
> > > {
> > > return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > > }
> > >
> > > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > > static struct attribute *kobj_attrs[] = {
> > > &sys_print_kobj_attr.attr,
> > > NULL
> > > };
> > >
> >
> > Hm, this seems like a kallsyms bug. IMO it should either fail the build
> > or omit the symbol from the kallsyms db. Truncating it seems dangerous
> > and counterintuitive.
> >
>
> Kallsyms will record all of the function name, without truncating it.
> But the kallsyms will return the truncated function name which is max to
> 127.
>
> > But regardless I really don't see a good reason to encourage this kind
> > of insanity in the livepatch code.
> >
>
> Yes, the above code is terrible, but we cannt stop user composing like
> that.
>
> Once the function name is like above, user will never have chance to use
> livepatch.
Again, this seems like a kallsyms bug. Fix the bug and the real world
need for this patch set goes away. The user will be forced to either
shorten their function name or increase KSYM_NAME_LEN.
--
Josh
On 04/14/15 at 12:11P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 01:03:48PM +0800, Minfei Huang wrote:
> > On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> > > On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > > > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > > > same, but the rest is not.
> > > > > >
> > > > > > Then function will never be patched, although function name and address
> > > > > > are provided both. The reason caused this bug is livepatch cannt
> > > > > > recognize the function name.
> > > > > >
> > > > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > > > and address, if provided. Once they are matched, we can confirm that the
> > > > > > patched function is found.
> > > > >
> > > > > From scripts/kallsyms.c:
> > > > >
> > > > > if (strlen(str) > KSYM_NAME_LEN) {
> > > > > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > > > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > > > str, strlen(str), KSYM_NAME_LEN);
> > > > > return -1;
> > > > > }
> > > > >
> > > > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > > > database in the first place.
> > > > >
> > > >
> > > > Actually, kernel allows overlength function name to be used. Following
> > > > is my testing module.
> > > >
> > > > We can got the address in /proc/kallsyms.
> > > > $ cat /proc/kallsyms | grep sysfs_print
> > > > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > ffffffffa0000010 t kobj_release [sysfs_print]
> > > > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > ffffffffa00004e0 b root_kobj [sysfs_print]
> > > > ffffffffa0000200 d print_ktype [sysfs_print]
> > > > ffffffffa00004a0 b print_kobj [sysfs_print]
> > > > ffffffffa000004c t sys_print_exit [sysfs_print]
> > > > ffffffffa0000144 r __func__.14514 [sysfs_print]
> > > > ffffffffa0000230 d kobj_attrs [sysfs_print]
> > > > ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> > > > ffffffffa0000260 d __this_module [sysfs_print]
> > > > ffffffffa000004c t cleanup_module [sysfs_print]
> > > >
> > > > Code:
> > > >
> > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > > > const char *buf, size_t count)
> > > > {
> > > > return count;
> > > > }
> > > >
> > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > > > struct kobj_attribute *attr, char *buf)
> > > > {
> > > > return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > > > }
> > > >
> > > > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > > > static struct attribute *kobj_attrs[] = {
> > > > &sys_print_kobj_attr.attr,
> > > > NULL
> > > > };
> > > >
> > >
> > > Hm, this seems like a kallsyms bug. IMO it should either fail the build
> > > or omit the symbol from the kallsyms db. Truncating it seems dangerous
> > > and counterintuitive.
> > >
> >
> > Kallsyms will record all of the function name, without truncating it.
> > But the kallsyms will return the truncated function name which is max to
> > 127.
> >
> > > But regardless I really don't see a good reason to encourage this kind
> > > of insanity in the livepatch code.
> > >
> >
> > Yes, the above code is terrible, but we cannt stop user composing like
> > that.
> >
> > Once the function name is like above, user will never have chance to use
> > livepatch.
>
> Again, this seems like a kallsyms bug. Fix the bug and the real world
> need for this patch set goes away. The user will be forced to either
> shorten their function name or increase KSYM_NAME_LEN.
>
kallsyms bug? I donot think increasing the KSYM_NAME_LEN is a good idea.
For end user, they may know litter about restriction of kallsyms and
livepatch. How can they know the restriction that function name is
limited to 127?
It is significant that livepatch supports to running specified kernel
and extra module, not only for deliverying kernel.
Thanks
Minfei
> --
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> On 04/14/15 at 12:11P, Josh Poimboeuf wrote:
> > On Tue, Apr 14, 2015 at 01:03:48PM +0800, Minfei Huang wrote:
> > > On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> > > > On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > > > > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > > > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > > > > same, but the rest is not.
> > > > > > >
> > > > > > > Then function will never be patched, although function name and address
> > > > > > > are provided both. The reason caused this bug is livepatch cannt
> > > > > > > recognize the function name.
> > > > > > >
> > > > > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > > > > and address, if provided. Once they are matched, we can confirm that the
> > > > > > > patched function is found.
> > > > > >
> > > > > > From scripts/kallsyms.c:
> > > > > >
> > > > > > if (strlen(str) > KSYM_NAME_LEN) {
> > > > > > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > > > > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > > > > str, strlen(str), KSYM_NAME_LEN);
> > > > > > return -1;
> > > > > > }
> > > > > >
> > > > > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > > > > database in the first place.
> > > > > >
> > > > >
> > > > > Actually, kernel allows overlength function name to be used. Following
> > > > > is my testing module.
> > > > >
> > > > > We can got the address in /proc/kallsyms.
> > > > > $ cat /proc/kallsyms | grep sysfs_print
> > > > > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > > ffffffffa0000010 t kobj_release [sysfs_print]
> > > > > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > > ffffffffa00004e0 b root_kobj [sysfs_print]
> > > > > ffffffffa0000200 d print_ktype [sysfs_print]
> > > > > ffffffffa00004a0 b print_kobj [sysfs_print]
> > > > > ffffffffa000004c t sys_print_exit [sysfs_print]
> > > > > ffffffffa0000144 r __func__.14514 [sysfs_print]
> > > > > ffffffffa0000230 d kobj_attrs [sysfs_print]
> > > > > ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> > > > > ffffffffa0000260 d __this_module [sysfs_print]
> > > > > ffffffffa000004c t cleanup_module [sysfs_print]
> > > > >
> > > > > Code:
> > > > >
> > > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > > > > const char *buf, size_t count)
> > > > > {
> > > > > return count;
> > > > > }
> > > > >
> > > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > > > > struct kobj_attribute *attr, char *buf)
> > > > > {
> > > > > return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > > > > }
> > > > >
> > > > > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > > > > static struct attribute *kobj_attrs[] = {
> > > > > &sys_print_kobj_attr.attr,
> > > > > NULL
> > > > > };
> > > > >
> > > >
> > > > Hm, this seems like a kallsyms bug. IMO it should either fail the build
> > > > or omit the symbol from the kallsyms db. Truncating it seems dangerous
> > > > and counterintuitive.
> > > >
> > >
> > > Kallsyms will record all of the function name, without truncating it.
> > > But the kallsyms will return the truncated function name which is max to
> > > 127.
> > >
> > > > But regardless I really don't see a good reason to encourage this kind
> > > > of insanity in the livepatch code.
> > > >
> > >
> > > Yes, the above code is terrible, but we cannt stop user composing like
> > > that.
> > >
> > > Once the function name is like above, user will never have chance to use
> > > livepatch.
> >
> > Again, this seems like a kallsyms bug. Fix the bug and the real world
> > need for this patch set goes away. The user will be forced to either
> > shorten their function name or increase KSYM_NAME_LEN.
> >
>
> kallsyms bug? I donot think increasing the KSYM_NAME_LEN is a good idea.
Well, neither is having a function name > KSYM_NAME_LEN :-)
>
> For end user, they may know litter about restriction of kallsyms and
> livepatch. How can they know the restriction that function name is
> limited to 127?
As I mentioned above, I think kallsyms.c should fail the build if it
encounters a symbol longer than KSYM_NAME_LEN.
--
Josh
On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > On 04/14/15 at 12:11P, Josh Poimboeuf wrote:
> > > On Tue, Apr 14, 2015 at 01:03:48PM +0800, Minfei Huang wrote:
> > > > On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> > > > > On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > > > > > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > > > > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > > > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > > > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > > > > > same, but the rest is not.
> > > > > > > >
> > > > > > > > Then function will never be patched, although function name and address
> > > > > > > > are provided both. The reason caused this bug is livepatch cannt
> > > > > > > > recognize the function name.
> > > > > > > >
> > > > > > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > > > > > and address, if provided. Once they are matched, we can confirm that the
> > > > > > > > patched function is found.
> > > > > > >
> > > > > > > From scripts/kallsyms.c:
> > > > > > >
> > > > > > > if (strlen(str) > KSYM_NAME_LEN) {
> > > > > > > fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > > > > > "Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > > > > > str, strlen(str), KSYM_NAME_LEN);
> > > > > > > return -1;
> > > > > > > }
> > > > > > >
> > > > > > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > > > > > database in the first place.
> > > > > > >
> > > > > >
> > > > > > Actually, kernel allows overlength function name to be used. Following
> > > > > > is my testing module.
> > > > > >
> > > > > > We can got the address in /proc/kallsyms.
> > > > > > $ cat /proc/kallsyms | grep sysfs_print
> > > > > > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > > > ffffffffa0000010 t kobj_release [sysfs_print]
> > > > > > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri [sysfs_print]
> > > > > > ffffffffa00004e0 b root_kobj [sysfs_print]
> > > > > > ffffffffa0000200 d print_ktype [sysfs_print]
> > > > > > ffffffffa00004a0 b print_kobj [sysfs_print]
> > > > > > ffffffffa000004c t sys_print_exit [sysfs_print]
> > > > > > ffffffffa0000144 r __func__.14514 [sysfs_print]
> > > > > > ffffffffa0000230 d kobj_attrs [sysfs_print]
> > > > > > ffffffffa0000240 d sys_print_kobj_attr [sysfs_print]
> > > > > > ffffffffa0000260 d __this_module [sysfs_print]
> > > > > > ffffffffa000004c t cleanup_module [sysfs_print]
> > > > > >
> > > > > > Code:
> > > > > >
> > > > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > > > > > const char *buf, size_t count)
> > > > > > {
> > > > > > return count;
> > > > > > }
> > > > > >
> > > > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > > > > > struct kobj_attribute *attr, char *buf)
> > > > > > {
> > > > > > return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > > > > > }
> > > > > >
> > > > > > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > > > > > static struct attribute *kobj_attrs[] = {
> > > > > > &sys_print_kobj_attr.attr,
> > > > > > NULL
> > > > > > };
> > > > > >
> > > > >
> > > > > Hm, this seems like a kallsyms bug. IMO it should either fail the build
> > > > > or omit the symbol from the kallsyms db. Truncating it seems dangerous
> > > > > and counterintuitive.
> > > > >
> > > >
> > > > Kallsyms will record all of the function name, without truncating it.
> > > > But the kallsyms will return the truncated function name which is max to
> > > > 127.
> > > >
> > > > > But regardless I really don't see a good reason to encourage this kind
> > > > > of insanity in the livepatch code.
> > > > >
> > > >
> > > > Yes, the above code is terrible, but we cannt stop user composing like
> > > > that.
> > > >
> > > > Once the function name is like above, user will never have chance to use
> > > > livepatch.
> > >
> > > Again, this seems like a kallsyms bug. Fix the bug and the real world
> > > need for this patch set goes away. The user will be forced to either
> > > shorten their function name or increase KSYM_NAME_LEN.
> > >
> >
> > kallsyms bug? I donot think increasing the KSYM_NAME_LEN is a good idea.
>
> Well, neither is having a function name > KSYM_NAME_LEN :-)
>
ohhhh...
Yes, The function name exceeding 127 is terrible.
> >
> > For end user, they may know litter about restriction of kallsyms and
> > livepatch. How can they know the restriction that function name is
> > limited to 127?
>
> As I mentioned above, I think kallsyms.c should fail the build if it
> encounters a symbol longer than KSYM_NAME_LEN.
>
I dont think it is a good idea to handle this case like that. The
function name is only for human recognization. Why the compiler fails
to build it?
Anyway, I will not insist for these patches, although I think it can
make livepatch robust.
> --
> Josh
On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > >
> > > For end user, they may know litter about restriction of kallsyms and
> > > livepatch. How can they know the restriction that function name is
> > > limited to 127?
> >
> > As I mentioned above, I think kallsyms.c should fail the build if it
> > encounters a symbol longer than KSYM_NAME_LEN.
> >
>
> I dont think it is a good idea to handle this case like that. The
> function name is only for human recognization. Why the compiler fails
> to build it?
Well, the function name isn't only for human recognition. kpatch-build
generates patch modules automatically. It assumes that the compiled
function name matches the kallsyms name. And I'd guess that a lot of
other code (both in-kernel and user space tools) make the same
assumption.
Not to mention that most humans would also make the same assumption...
--
Josh
On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > >
> > > > For end user, they may know litter about restriction of kallsyms and
> > > > livepatch. How can they know the restriction that function name is
> > > > limited to 127?
> > >
> > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > encounters a symbol longer than KSYM_NAME_LEN.
> > >
> >
> > I dont think it is a good idea to handle this case like that. The
> > function name is only for human recognization. Why the compiler fails
> > to build it?
>
> Well, the function name isn't only for human recognition. kpatch-build
> generates patch modules automatically. It assumes that the compiled
> function name matches the kallsyms name. And I'd guess that a lot of
> other code (both in-kernel and user space tools) make the same
> assumption.
>
> Not to mention that most humans would also make the same assumption...
Yes. The assumption is correct for most case.
It is significance for livepatch to support extra module, because in my
opinion kernel is more stable than the third module.
So it is more important, if the livepatch can patch all sorts of patch.
For dynamic function name, I think it is simple to avoid it.
Usually, we will use ominity to handle a bunch of machines. So it is
simple, if we use script to get the function address and build the patch.
Josh, is there any chance to accept my patches? It may be important
somewhile that system can not restart without schedule to reload the
fixed-module.
Thanks
Minfei
>
> --
> Josh
On Tue 2015-04-14 23:55:36, Minfei Huang wrote:
> On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> > On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > > >
> > > > > For end user, they may know litter about restriction of kallsyms and
> > > > > livepatch. How can they know the restriction that function name is
> > > > > limited to 127?
> > > >
> > > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > > encounters a symbol longer than KSYM_NAME_LEN.
> > > >
> > >
> > > I dont think it is a good idea to handle this case like that. The
> > > function name is only for human recognization. Why the compiler fails
> > > to build it?
> >
> > Well, the function name isn't only for human recognition. kpatch-build
> > generates patch modules automatically. It assumes that the compiled
> > function name matches the kallsyms name. And I'd guess that a lot of
> > other code (both in-kernel and user space tools) make the same
> > assumption.
> >
> > Not to mention that most humans would also make the same assumption...
>
> Yes. The assumption is correct for most case.
>
> It is significance for livepatch to support extra module, because in my
> opinion kernel is more stable than the third module.
>
> So it is more important, if the livepatch can patch all sorts of patch.
> For dynamic function name, I think it is simple to avoid it.
Do you have some really existing module with such a crazy long
function names or is this debate pure theoretical, please?
Also have you tested your patch and tried to apply livepatch
for some really exiting module, please? I ask because it won't
be trivial to create such a patch. Also the patch would work
only for the one running system.
Best Regards,
Petr
> Usually, we will use ominity to handle a bunch of machines. So it is
> simple, if we use script to get the function address and build the patch.
>
> Josh, is there any chance to accept my patches? It may be important
> somewhile that system can not restart without schedule to reload the
> fixed-module.
>
> Thanks
> Minfei
>
> >
> > --
> > Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/14/15 at 06:27pm, Petr Mladek wrote:
> On Tue 2015-04-14 23:55:36, Minfei Huang wrote:
> > On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> > > On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > > > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > > > >
> > > > > > For end user, they may know litter about restriction of kallsyms and
> > > > > > livepatch. How can they know the restriction that function name is
> > > > > > limited to 127?
> > > > >
> > > > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > > > encounters a symbol longer than KSYM_NAME_LEN.
> > > > >
> > > >
> > > > I dont think it is a good idea to handle this case like that. The
> > > > function name is only for human recognization. Why the compiler fails
> > > > to build it?
> > >
> > > Well, the function name isn't only for human recognition. kpatch-build
> > > generates patch modules automatically. It assumes that the compiled
> > > function name matches the kallsyms name. And I'd guess that a lot of
> > > other code (both in-kernel and user space tools) make the same
> > > assumption.
> > >
> > > Not to mention that most humans would also make the same assumption...
> >
> > Yes. The assumption is correct for most case.
> >
> > It is significance for livepatch to support extra module, because in my
> > opinion kernel is more stable than the third module.
> >
> > So it is more important, if the livepatch can patch all sorts of patch.
> > For dynamic function name, I think it is simple to avoid it.
>
> Do you have some really existing module with such a crazy long
> function names or is this debate pure theoretical, please?
>
No, I do not have such running module which function name is exceed to
127.
Again, we can not predict what end user do to name the function name. I
think the overlength function name is valid for linux kernel, if the
module can be installed.
> Also have you tested your patch and tried to apply livepatch
> for some really exiting module, please? I ask because it won't
The patched livepatch works well for my testing module which has the
overlength name function.
Thanks
Minfei
> be trivial to create such a patch. Also the patch would work
> only for the one running system.
>
> Best Regards,
> Petr
>
> > Usually, we will use ominity to handle a bunch of machines. So it is
> > simple, if we use script to get the function address and build the patch.
> >
> > Josh, is there any chance to accept my patches? It may be important
> > somewhile that system can not restart without schedule to reload the
> > fixed-module.
> >
> > Thanks
> > Minfei
> >
> > >
> > > --
> > > Josh
> > --
> > To unsubscribe from this list: send the line "unsubscribe live-patching" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Wed 2015-04-15 01:01:39, Minfei Huang wrote:
> On 04/14/15 at 06:27pm, Petr Mladek wrote:
> > On Tue 2015-04-14 23:55:36, Minfei Huang wrote:
> > > On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> > > > On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > > > > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > > > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > > > > >
> > > > > > > For end user, they may know litter about restriction of kallsyms and
> > > > > > > livepatch. How can they know the restriction that function name is
> > > > > > > limited to 127?
> > > > > >
> > > > > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > > > > encounters a symbol longer than KSYM_NAME_LEN.
> > > > > >
> > > > >
> > > > > I dont think it is a good idea to handle this case like that. The
> > > > > function name is only for human recognization. Why the compiler fails
> > > > > to build it?
> > > >
> > > > Well, the function name isn't only for human recognition. kpatch-build
> > > > generates patch modules automatically. It assumes that the compiled
> > > > function name matches the kallsyms name. And I'd guess that a lot of
> > > > other code (both in-kernel and user space tools) make the same
> > > > assumption.
> > > >
> > > > Not to mention that most humans would also make the same assumption...
> > >
> > > Yes. The assumption is correct for most case.
> > >
> > > It is significance for livepatch to support extra module, because in my
> > > opinion kernel is more stable than the third module.
> > >
> > > So it is more important, if the livepatch can patch all sorts of patch.
> > > For dynamic function name, I think it is simple to avoid it.
> >
> > Do you have some really existing module with such a crazy long
> > function names or is this debate pure theoretical, please?
> >
>
> No, I do not have such running module which function name is exceed to
> 127.
>
> Again, we can not predict what end user do to name the function name. I
> think the overlength function name is valid for linux kernel, if the
> module can be installed.
My position on this is that using >127 length function names is
insane. I would be scared to use such a module on a production system.
If we refuse patching, we actually do a favor for the user.
Instead of fixing live patch for such a scenario, we should suggest
the user to use more trustful modules.
Best Regards,
Petr
On 04/14/15 at 08:41pm, Petr Mladek wrote:
> On Wed 2015-04-15 01:01:39, Minfei Huang wrote:
> > On 04/14/15 at 06:27pm, Petr Mladek wrote:
> > > On Tue 2015-04-14 23:55:36, Minfei Huang wrote:
> > > > On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> > > > > On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > > > > > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > > > > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > > > > > >
> > > > > > > > For end user, they may know litter about restriction of kallsyms and
> > > > > > > > livepatch. How can they know the restriction that function name is
> > > > > > > > limited to 127?
> > > > > > >
> > > > > > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > > > > > encounters a symbol longer than KSYM_NAME_LEN.
> > > > > > >
> > > > > >
> > > > > > I dont think it is a good idea to handle this case like that. The
> > > > > > function name is only for human recognization. Why the compiler fails
> > > > > > to build it?
> > > > >
> > > > > Well, the function name isn't only for human recognition. kpatch-build
> > > > > generates patch modules automatically. It assumes that the compiled
> > > > > function name matches the kallsyms name. And I'd guess that a lot of
> > > > > other code (both in-kernel and user space tools) make the same
> > > > > assumption.
> > > > >
> > > > > Not to mention that most humans would also make the same assumption...
> > > >
> > > > Yes. The assumption is correct for most case.
> > > >
> > > > It is significance for livepatch to support extra module, because in my
> > > > opinion kernel is more stable than the third module.
> > > >
> > > > So it is more important, if the livepatch can patch all sorts of patch.
> > > > For dynamic function name, I think it is simple to avoid it.
> > >
> > > Do you have some really existing module with such a crazy long
> > > function names or is this debate pure theoretical, please?
> > >
> >
> > No, I do not have such running module which function name is exceed to
> > 127.
> >
> > Again, we can not predict what end user do to name the function name. I
> > think the overlength function name is valid for linux kernel, if the
> > module can be installed.
>
> My position on this is that using >127 length function names is
> insane. I would be scared to use such a module on a production system.
> If we refuse patching, we actually do a favor for the user.
> Instead of fixing live patch for such a scenario, we should suggest
> the user to use more trustful modules.
Yes, the function name can be changed, before the extra module is
installed to the production system.
We discuss around and around, there are still some confusion with it.
1) How does end user know that livepatch can _not_ support the function
which length is larger than 127. We can not enforce the end user
to know the livepatch and kallsyms code in detail.
2) How does end user use livepatch to patch running extra module, once
the module is running in the production system, if the function name
is insane.
3) The error message is ambiguity, if we try to patch the overlength
function. We can give the error message clearly, once the function
name is overlength.
I think it is better that we can take more time on the people who will
use livepatch frequently.
Attaching a patch to make error message explictly for the overlength
function name.
>From d46a230499303657a914d6939c3afbeff906796c Mon Sep 17 00:00:00 2001
From: Minfei Huang <[email protected]>
Date: Wed, 15 Apr 2015 10:02:43 +0800
Subject: [PATCH] livepatch: Make error message explicitly for the overlength
function name
For not, livepatch do not support the function which name is larger than
KSYM_NAME_LEN-1. It may be confusion user with error message
"livepatch: symbol 'xxx(function name)' not found in symbol table".
Make error message explicitly for overlength issue.
Signed-off-by: Minfei Huang <[email protected]>
---
kernel/livepatch/core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3f9f1d6..d1f2404 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -789,6 +789,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
return -ENOMEM;
for (func = obj->funcs; func->old_name; func++) {
+ if (strlen(func->old_name) > (KSYM_NAME_LEN-1)) {
+ pr_err("%s is overlength, the max to be supported is %d\n",
+ func->old_name, KSYM_NAME_LEN-1);
+ ret = -EINVAL;
+ goto free;
+ }
ret = klp_init_func(obj, func);
if (ret)
goto free;
--
2.2.2
>
> Best Regards,
> Petr
On Wed, 15 Apr 2015, Minfei Huang wrote:
> On 04/14/15 at 08:41pm, Petr Mladek wrote:
> > On Wed 2015-04-15 01:01:39, Minfei Huang wrote:
> > > On 04/14/15 at 06:27pm, Petr Mladek wrote:
> > > > On Tue 2015-04-14 23:55:36, Minfei Huang wrote:
> > > > > On 04/14/15 at 10:11P, Josh Poimboeuf wrote:
> > > > > > On Tue, Apr 14, 2015 at 01:45:49PM +0800, Minfei Huang wrote:
> > > > > > > On 04/14/15 at 12:32P, Josh Poimboeuf wrote:
> > > > > > > > On Tue, Apr 14, 2015 at 01:29:50PM +0800, Minfei Huang wrote:
> > > > > > > > >
> > > > > > > > > For end user, they may know litter about restriction of kallsyms and
> > > > > > > > > livepatch. How can they know the restriction that function name is
> > > > > > > > > limited to 127?
> > > > > > > >
> > > > > > > > As I mentioned above, I think kallsyms.c should fail the build if it
> > > > > > > > encounters a symbol longer than KSYM_NAME_LEN.
> > > > > > > >
> > > > > > >
> > > > > > > I dont think it is a good idea to handle this case like that. The
> > > > > > > function name is only for human recognization. Why the compiler fails
> > > > > > > to build it?
> > > > > >
> > > > > > Well, the function name isn't only for human recognition. kpatch-build
> > > > > > generates patch modules automatically. It assumes that the compiled
> > > > > > function name matches the kallsyms name. And I'd guess that a lot of
> > > > > > other code (both in-kernel and user space tools) make the same
> > > > > > assumption.
> > > > > >
> > > > > > Not to mention that most humans would also make the same assumption...
> > > > >
> > > > > Yes. The assumption is correct for most case.
> > > > >
> > > > > It is significance for livepatch to support extra module, because in my
> > > > > opinion kernel is more stable than the third module.
> > > > >
> > > > > So it is more important, if the livepatch can patch all sorts of patch.
> > > > > For dynamic function name, I think it is simple to avoid it.
> > > >
> > > > Do you have some really existing module with such a crazy long
> > > > function names or is this debate pure theoretical, please?
> > > >
> > >
> > > No, I do not have such running module which function name is exceed to
> > > 127.
> > >
> > > Again, we can not predict what end user do to name the function name. I
> > > think the overlength function name is valid for linux kernel, if the
> > > module can be installed.
> >
> > My position on this is that using >127 length function names is
> > insane. I would be scared to use such a module on a production system.
> > If we refuse patching, we actually do a favor for the user.
> > Instead of fixing live patch for such a scenario, we should suggest
> > the user to use more trustful modules.
>
> Yes, the function name can be changed, before the extra module is
> installed to the production system.
>
> We discuss around and around, there are still some confusion with it.
>
> 1) How does end user know that livepatch can _not_ support the function
> which length is larger than 127. We can not enforce the end user
> to know the livepatch and kallsyms code in detail.
> 2) How does end user use livepatch to patch running extra module, once
> the module is running in the production system, if the function name
> is insane.
> 3) The error message is ambiguity, if we try to patch the overlength
> function. We can give the error message clearly, once the function
> name is overlength.
>
> I think it is better that we can take more time on the people who will
> use livepatch frequently.
Just my two cents, even if we admit that such change is worth it (and I
am still not convinced that it is the case), I think it would make sense
to fix it somewhere in kallsyms as Josh proposed. I suspect that when
function names longer than KSYM_NAME_LEN were common there would be many
similar problems elsewhere in the kernel.
That is you can prepare a patch to kallsyms and submit it there. Not sure
who is the maintainer but he might have an opinion about this...
Thanks,
Miroslav
>
> Attaching a patch to make error message explictly for the overlength
> function name.
>
> >From d46a230499303657a914d6939c3afbeff906796c Mon Sep 17 00:00:00 2001
> From: Minfei Huang <[email protected]>
> Date: Wed, 15 Apr 2015 10:02:43 +0800
> Subject: [PATCH] livepatch: Make error message explicitly for the overlength
> function name
>
> For not, livepatch do not support the function which name is larger than
> KSYM_NAME_LEN-1. It may be confusion user with error message
> "livepatch: symbol 'xxx(function name)' not found in symbol table".
>
> Make error message explicitly for overlength issue.
>
> Signed-off-by: Minfei Huang <[email protected]>
> ---
> kernel/livepatch/core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 3f9f1d6..d1f2404 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -789,6 +789,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return -ENOMEM;
>
> for (func = obj->funcs; func->old_name; func++) {
> + if (strlen(func->old_name) > (KSYM_NAME_LEN-1)) {
> + pr_err("%s is overlength, the max to be supported is %d\n",
> + func->old_name, KSYM_NAME_LEN-1);
> + ret = -EINVAL;
> + goto free;
> + }
> ret = klp_init_func(obj, func);
> if (ret)
> goto free;
> --
> 2.2.2
>
> >
> > Best Regards,
> > Petr
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 04/15/15 at 10:30P, Miroslav Benes wrote:
> On Wed, 15 Apr 2015, Minfei Huang wrote:
> >
> > Yes, the function name can be changed, before the extra module is
> > installed to the production system.
> >
> > We discuss around and around, there are still some confusion with it.
> >
> > 1) How does end user know that livepatch can _not_ support the function
> > which length is larger than 127. We can not enforce the end user
> > to know the livepatch and kallsyms code in detail.
> > 2) How does end user use livepatch to patch running extra module, once
> > the module is running in the production system, if the function name
> > is insane.
> > 3) The error message is ambiguity, if we try to patch the overlength
> > function. We can give the error message clearly, once the function
> > name is overlength.
> >
> > I think it is better that we can take more time on the people who will
> > use livepatch frequently.
>
> Just my two cents, even if we admit that such change is worth it (and I
> am still not convinced that it is the case), I think it would make sense
> to fix it somewhere in kallsyms as Josh proposed. I suspect that when
> function names longer than KSYM_NAME_LEN were common there would be many
> similar problems elsewhere in the kernel.
>
> That is you can prepare a patch to kallsyms and submit it there. Not sure
> who is the maintainer but he might have an opinion about this...
>
> Thanks,
> Miroslav
Thanks all who help to review these patches.
I will not insist on it, since it may be encountered rarely.
Thanks
Minfei
On 04/15/15 at 10:30P, Miroslav Benes wrote:
> On Wed, 15 Apr 2015, Minfei Huang wrote:
>
> >
> > Yes, the function name can be changed, before the extra module is
> > installed to the production system.
> >
> > We discuss around and around, there are still some confusion with it.
> >
> > 1) How does end user know that livepatch can _not_ support the function
> > which length is larger than 127. We can not enforce the end user
> > to know the livepatch and kallsyms code in detail.
> > 2) How does end user use livepatch to patch running extra module, once
> > the module is running in the production system, if the function name
> > is insane.
> > 3) The error message is ambiguity, if we try to patch the overlength
> > function. We can give the error message clearly, once the function
> > name is overlength.
> >
> > I think it is better that we can take more time on the people who will
> > use livepatch frequently.
>
> Just my two cents, even if we admit that such change is worth it (and I
> am still not convinced that it is the case), I think it would make sense
> to fix it somewhere in kallsyms as Josh proposed. I suspect that when
Ohhh...
Fixing kallsyms to restrict the function name length maybe is not a good
idea. I have no idea how we should do this, except for the coding
problems.
> function names longer than KSYM_NAME_LEN were common there would be many
> similar problems elsewhere in the kernel.
>
> That is you can prepare a patch to kallsyms and submit it there. Not sure
> who is the maintainer but he might have an opinion about this...
>
> Thanks,
> Miroslav
Hold on, I get a scenario that livepatch may do fatal error. I am fine
that livepatch do not support overlength function name, because it can
not corrupt the kernel.
Once there is a function name A is larger than 127, and another function
name B is as longer as 127, it is disaster that we want to patch
function B, if function name of first 127 is same between A and B.
Livepatch may find the function of A to patch it. So this patch(2/2) may
be needed to fix the issue.
Thanks
Minfei
On Wed, 15 Apr 2015, Minfei Huang wrote:
> On 04/15/15 at 10:30P, Miroslav Benes wrote:
> > On Wed, 15 Apr 2015, Minfei Huang wrote:
> >
> > >
> > > Yes, the function name can be changed, before the extra module is
> > > installed to the production system.
> > >
> > > We discuss around and around, there are still some confusion with it.
> > >
> > > 1) How does end user know that livepatch can _not_ support the function
> > > which length is larger than 127. We can not enforce the end user
> > > to know the livepatch and kallsyms code in detail.
> > > 2) How does end user use livepatch to patch running extra module, once
> > > the module is running in the production system, if the function name
> > > is insane.
> > > 3) The error message is ambiguity, if we try to patch the overlength
> > > function. We can give the error message clearly, once the function
> > > name is overlength.
> > >
> > > I think it is better that we can take more time on the people who will
> > > use livepatch frequently.
> >
> > Just my two cents, even if we admit that such change is worth it (and I
> > am still not convinced that it is the case), I think it would make sense
> > to fix it somewhere in kallsyms as Josh proposed. I suspect that when
>
> Ohhh...
>
> Fixing kallsyms to restrict the function name length maybe is not a good
> idea. I have no idea how we should do this, except for the coding
> problems.
Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We
apparently do not do it when kernel modules are built out of the tree (as
you demonstrated before). So the question is whether we should do it also
there. That is one thing we try to tell you.
The other one is that 128 characters long function names are insane.
Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you
could even try to add the check to checkpatch.pl.
> > function names longer than KSYM_NAME_LEN were common there would be many
> > similar problems elsewhere in the kernel.
> >
> > That is you can prepare a patch to kallsyms and submit it there. Not sure
> > who is the maintainer but he might have an opinion about this...
> >
> > Thanks,
> > Miroslav
>
> Hold on, I get a scenario that livepatch may do fatal error. I am fine
> that livepatch do not support overlength function name, because it can
> not corrupt the kernel.
>
> Once there is a function name A is larger than 127, and another function
> name B is as longer as 127, it is disaster that we want to patch
> function B, if function name of first 127 is same between A and B.
True, but see above.
> Livepatch may find the function of A to patch it. So this patch(2/2) may
> be needed to fix the issue.
Hm, but this patch is not the solution for the issue, or is it? You would
check only those first KSYM_NAME_LEN characters, but that would not
differentiate between A and B. Or maybe I do not follow.
Thanks
Miroslav
I agree that 128+ function names are bad and there is no need for such
long names, epically for compatibility with 80 char/line displays. I
cannot think of an example even near as long as 127 characters. Are
there even such long function names anywhere in the kernel?
Justin
On Wed, Apr 15, 2015 at 7:58 AM, Miroslav Benes <[email protected]> wrote:
> On Wed, 15 Apr 2015, Minfei Huang wrote:
>
>> On 04/15/15 at 10:30P, Miroslav Benes wrote:
>> > On Wed, 15 Apr 2015, Minfei Huang wrote:
>> >
>> > >
>> > > Yes, the function name can be changed, before the extra module is
>> > > installed to the production system.
>> > >
>> > > We discuss around and around, there are still some confusion with it.
>> > >
>> > > 1) How does end user know that livepatch can _not_ support the function
>> > > which length is larger than 127. We can not enforce the end user
>> > > to know the livepatch and kallsyms code in detail.
>> > > 2) How does end user use livepatch to patch running extra module, once
>> > > the module is running in the production system, if the function name
>> > > is insane.
>> > > 3) The error message is ambiguity, if we try to patch the overlength
>> > > function. We can give the error message clearly, once the function
>> > > name is overlength.
>> > >
>> > > I think it is better that we can take more time on the people who will
>> > > use livepatch frequently.
>> >
>> > Just my two cents, even if we admit that such change is worth it (and I
>> > am still not convinced that it is the case), I think it would make sense
>> > to fix it somewhere in kallsyms as Josh proposed. I suspect that when
>>
>> Ohhh...
>>
>> Fixing kallsyms to restrict the function name length maybe is not a good
>> idea. I have no idea how we should do this, except for the coding
>> problems.
>
> Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We
> apparently do not do it when kernel modules are built out of the tree (as
> you demonstrated before). So the question is whether we should do it also
> there. That is one thing we try to tell you.
>
> The other one is that 128 characters long function names are insane.
> Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you
> could even try to add the check to checkpatch.pl.
>
>> > function names longer than KSYM_NAME_LEN were common there would be many
>> > similar problems elsewhere in the kernel.
>> >
>> > That is you can prepare a patch to kallsyms and submit it there. Not sure
>> > who is the maintainer but he might have an opinion about this...
>> >
>> > Thanks,
>> > Miroslav
>>
>> Hold on, I get a scenario that livepatch may do fatal error. I am fine
>> that livepatch do not support overlength function name, because it can
>> not corrupt the kernel.
>>
>> Once there is a function name A is larger than 127, and another function
>> name B is as longer as 127, it is disaster that we want to patch
>> function B, if function name of first 127 is same between A and B.
>
> True, but see above.
>
>> Livepatch may find the function of A to patch it. So this patch(2/2) may
>> be needed to fix the issue.
>
> Hm, but this patch is not the solution for the issue, or is it? You would
> check only those first KSYM_NAME_LEN characters, but that would not
> differentiate between A and B. Or maybe I do not follow.
>
> Thanks
> Miroslav
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 04/15/15 at 12:24pm, Justin Keller wrote:
> I agree that 128+ function names are bad and there is no need for such
> long names, epically for compatibility with 80 char/line displays. I
> cannot think of an example even near as long as 127 characters. Are
> there even such long function names anywhere in the kernel?
>
> Justin
>
Yes, the overlength function name is insane. For the upstream source, we
can never meet such case, because we can control the code to be merged
or not.
But there is an exception for extra module which composes by end user.
IMO, livepatch is a tool which helps to inject new function to replace
the old one. Since the patahed patch is from userspace, it may be not
trusted. Do all of the condition judgement we can to confirm it does
not corrupt the kernel.
Thanks
Minfei
> On Wed, Apr 15, 2015 at 7:58 AM, Miroslav Benes <[email protected]> wrote:
> > On Wed, 15 Apr 2015, Minfei Huang wrote:
> >
> >> On 04/15/15 at 10:30P, Miroslav Benes wrote:
> >> > On Wed, 15 Apr 2015, Minfei Huang wrote:
> >> >
> >> > >
> >> > > Yes, the function name can be changed, before the extra module is
> >> > > installed to the production system.
> >> > >
> >> > > We discuss around and around, there are still some confusion with it.
> >> > >
> >> > > 1) How does end user know that livepatch can _not_ support the function
> >> > > which length is larger than 127. We can not enforce the end user
> >> > > to know the livepatch and kallsyms code in detail.
> >> > > 2) How does end user use livepatch to patch running extra module, once
> >> > > the module is running in the production system, if the function name
> >> > > is insane.
> >> > > 3) The error message is ambiguity, if we try to patch the overlength
> >> > > function. We can give the error message clearly, once the function
> >> > > name is overlength.
> >> > >
> >> > > I think it is better that we can take more time on the people who will
> >> > > use livepatch frequently.
> >> >
> >> > Just my two cents, even if we admit that such change is worth it (and I
> >> > am still not convinced that it is the case), I think it would make sense
> >> > to fix it somewhere in kallsyms as Josh proposed. I suspect that when
> >>
> >> Ohhh...
> >>
> >> Fixing kallsyms to restrict the function name length maybe is not a good
> >> idea. I have no idea how we should do this, except for the coding
> >> problems.
> >
> > Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We
> > apparently do not do it when kernel modules are built out of the tree (as
> > you demonstrated before). So the question is whether we should do it also
> > there. That is one thing we try to tell you.
> >
> > The other one is that 128 characters long function names are insane.
> > Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you
> > could even try to add the check to checkpatch.pl.
> >
> >> > function names longer than KSYM_NAME_LEN were common there would be many
> >> > similar problems elsewhere in the kernel.
> >> >
> >> > That is you can prepare a patch to kallsyms and submit it there. Not sure
> >> > who is the maintainer but he might have an opinion about this...
> >> >
> >> > Thanks,
> >> > Miroslav
> >>
> >> Hold on, I get a scenario that livepatch may do fatal error. I am fine
> >> that livepatch do not support overlength function name, because it can
> >> not corrupt the kernel.
> >>
> >> Once there is a function name A is larger than 127, and another function
> >> name B is as longer as 127, it is disaster that we want to patch
> >> function B, if function name of first 127 is same between A and B.
> >
> > True, but see above.
> >
> >> Livepatch may find the function of A to patch it. So this patch(2/2) may
> >> be needed to fix the issue.
> >
> > Hm, but this patch is not the solution for the issue, or is it? You would
> > check only those first KSYM_NAME_LEN characters, but that would not
> > differentiate between A and B. Or maybe I do not follow.
> >
> > Thanks
> > Miroslav
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
On 04/15/15 at 01:58P, Miroslav Benes wrote:
> On Wed, 15 Apr 2015, Minfei Huang wrote:
>
> > On 04/15/15 at 10:30P, Miroslav Benes wrote:
> > > On Wed, 15 Apr 2015, Minfei Huang wrote:
> > >
> > > >
> > > > Yes, the function name can be changed, before the extra module is
> > > > installed to the production system.
> > > >
> > > > We discuss around and around, there are still some confusion with it.
> > > >
> > > > 1) How does end user know that livepatch can _not_ support the function
> > > > which length is larger than 127. We can not enforce the end user
> > > > to know the livepatch and kallsyms code in detail.
> > > > 2) How does end user use livepatch to patch running extra module, once
> > > > the module is running in the production system, if the function name
> > > > is insane.
> > > > 3) The error message is ambiguity, if we try to patch the overlength
> > > > function. We can give the error message clearly, once the function
> > > > name is overlength.
> > > >
> > > > I think it is better that we can take more time on the people who will
> > > > use livepatch frequently.
> > >
> > > Just my two cents, even if we admit that such change is worth it (and I
> > > am still not convinced that it is the case), I think it would make sense
> > > to fix it somewhere in kallsyms as Josh proposed. I suspect that when
> >
> > Ohhh...
> >
> > Fixing kallsyms to restrict the function name length maybe is not a good
> > idea. I have no idea how we should do this, except for the coding
> > problems.
>
> Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We
> apparently do not do it when kernel modules are built out of the tree (as
> you demonstrated before). So the question is whether we should do it also
> there. That is one thing we try to tell you.
>
> The other one is that 128 characters long function names are insane.
> Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you
> could even try to add the check to checkpatch.pl.
>
> > > function names longer than KSYM_NAME_LEN were common there would be many
> > > similar problems elsewhere in the kernel.
> > >
> > > That is you can prepare a patch to kallsyms and submit it there. Not sure
> > > who is the maintainer but he might have an opinion about this...
> > >
> > > Thanks,
> > > Miroslav
> >
> > Hold on, I get a scenario that livepatch may do fatal error. I am fine
> > that livepatch do not support overlength function name, because it can
> > not corrupt the kernel.
> >
> > Once there is a function name A is larger than 127, and another function
> > name B is as longer as 127, it is disaster that we want to patch
> > function B, if function name of first 127 is same between A and B.
>
> True, but see above.
>
> > Livepatch may find the function of A to patch it. So this patch(2/2) may
> > be needed to fix the issue.
>
> Hm, but this patch is not the solution for the issue, or is it? You would
> check only those first KSYM_NAME_LEN characters, but that would not
> differentiate between A and B. Or maybe I do not follow.
>
Hello, guys.
Do I need to post a patch to fix the above issue? Applied following
patch, livepatch will fail to patch the patch, since there are more than
two symbols to be matched.
If so, I would like to post an official patch to the maillist.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..67b237f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -152,7 +152,7 @@ static int klp_find_callback(void *data, const char *name,
if ((mod && !args->objname) || (!mod && args->objname))
return 0;
- if (strcmp(args->name, name))
+ if (strncmp(args->name, name, KSYM_NAME_LEN-1))
return 0;
if (args->objname && strcmp(args->objname, mod->name))
Thanks
Minfei
> Thanks
> Miroslav
On Sun, 26 Apr 2015, Minfei Huang wrote:
> On 04/15/15 at 01:58P, Miroslav Benes wrote:
> > On Wed, 15 Apr 2015, Minfei Huang wrote:
> >
> > > On 04/15/15 at 10:30P, Miroslav Benes wrote:
> > > > On Wed, 15 Apr 2015, Minfei Huang wrote:
> > > >
> > > > >
> > > > > Yes, the function name can be changed, before the extra module is
> > > > > installed to the production system.
> > > > >
> > > > > We discuss around and around, there are still some confusion with it.
> > > > >
> > > > > 1) How does end user know that livepatch can _not_ support the function
> > > > > which length is larger than 127. We can not enforce the end user
> > > > > to know the livepatch and kallsyms code in detail.
> > > > > 2) How does end user use livepatch to patch running extra module, once
> > > > > the module is running in the production system, if the function name
> > > > > is insane.
> > > > > 3) The error message is ambiguity, if we try to patch the overlength
> > > > > function. We can give the error message clearly, once the function
> > > > > name is overlength.
> > > > >
> > > > > I think it is better that we can take more time on the people who will
> > > > > use livepatch frequently.
> > > >
> > > > Just my two cents, even if we admit that such change is worth it (and I
> > > > am still not convinced that it is the case), I think it would make sense
> > > > to fix it somewhere in kallsyms as Josh proposed. I suspect that when
> > >
> > > Ohhh...
> > >
> > > Fixing kallsyms to restrict the function name length maybe is not a good
> > > idea. I have no idea how we should do this, except for the coding
> > > problems.
> >
> > Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We
> > apparently do not do it when kernel modules are built out of the tree (as
> > you demonstrated before). So the question is whether we should do it also
> > there. That is one thing we try to tell you.
> >
> > The other one is that 128 characters long function names are insane.
> > Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you
> > could even try to add the check to checkpatch.pl.
> >
> > > > function names longer than KSYM_NAME_LEN were common there would be many
> > > > similar problems elsewhere in the kernel.
> > > >
> > > > That is you can prepare a patch to kallsyms and submit it there. Not sure
> > > > who is the maintainer but he might have an opinion about this...
> > > >
> > > > Thanks,
> > > > Miroslav
> > >
> > > Hold on, I get a scenario that livepatch may do fatal error. I am fine
> > > that livepatch do not support overlength function name, because it can
> > > not corrupt the kernel.
> > >
> > > Once there is a function name A is larger than 127, and another function
> > > name B is as longer as 127, it is disaster that we want to patch
> > > function B, if function name of first 127 is same between A and B.
> >
> > True, but see above.
> >
> > > Livepatch may find the function of A to patch it. So this patch(2/2) may
> > > be needed to fix the issue.
> >
> > Hm, but this patch is not the solution for the issue, or is it? You would
> > check only those first KSYM_NAME_LEN characters, but that would not
> > differentiate between A and B. Or maybe I do not follow.
> >
>
> Hello, guys.
>
> Do I need to post a patch to fix the above issue? Applied following
> patch, livepatch will fail to patch the patch, since there are more than
> two symbols to be matched.
> If so, I would like to post an official patch to the maillist.
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..67b237f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -152,7 +152,7 @@ static int klp_find_callback(void *data, const char *name,
> if ((mod && !args->objname) || (!mod && args->objname))
> return 0;
>
> - if (strcmp(args->name, name))
> + if (strncmp(args->name, name, KSYM_NAME_LEN-1))
> return 0;
>
> if (args->objname && strcmp(args->objname, mod->name))
This means that in your scenario described above count would be >0 here
and kallsyms symbol would not be resolved... which is the same situation
as of now without your patch. And you can find this objection above as
well.
I still think this needs to be fixed somewhere else and you can find hints
and points in the thread.
Maybe someone else feels differently and will say so...
Cheers,
Miroslav