[PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

In order to restrict the patch, we can verify the provided function
address and name match. Now we have can only verify the vmlinux function
name and address.

Add a new function to verify extra module function name and address. The
patch would not be patched, if the function name and address are not
matched.

Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>
---
 kernel/livepatch/core.c | 54 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3f9f1d6..ff42c29 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -196,12 +196,16 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 }
 
 struct klp_verify_args {
+	const char *objname;
 	const char *name;
 	const unsigned long addr;
 };
 
-static int klp_verify_callback(void *data, const char *name,
-			       struct module *mod, unsigned long addr)
+typedef int (*klp_verify_callback)(void *data, const char *name,
+		struct module *mod, unsigned long addr);
+
+static int klp_verify_vmlinux_callback(void *data, const char *name,
+		struct module *mod, unsigned long addr)
 {
 	struct klp_verify_args *args = data;
 
@@ -213,18 +217,38 @@ static int klp_verify_callback(void *data, const char *name,
 	return 0;
 }
 
-static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
+static int klp_verify_module_callback(void *data, const char *name,
+		struct module *mod, unsigned long addr)
+{
+	struct klp_verify_args *args = data;
+
+	if (!mod || !args->objname)
+		return 0;
+
+	if (!strcmp(args->objname, mod->name) &&
+	    !strcmp(args->name, name) &&
+	    args->addr == addr)
+		return 1;
+
+	return 0;
+}
+
+static int klp_verify_symbol(const char *objname, const char *name,
+		unsigned long addr)
 {
 	struct klp_verify_args args = {
+		.objname = objname,
 		.name = name,
 		.addr = addr,
 	};
+	klp_verify_callback fn = objname ?
+		klp_verify_module_callback : klp_verify_vmlinux_callback;
 
-	if (kallsyms_on_each_symbol(klp_verify_callback, &args))
+	if (kallsyms_on_each_symbol(fn, &args))
 		return 0;
 
-	pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
-		name, addr);
+	pr_err("symbol '%s' not found at specified address 0x%016lx, %s mismatch?\n",
+			name, addr, objname ? objname : "kernel");
 	return -EINVAL;
 }
 
@@ -238,12 +262,12 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
 	func->old_addr = 0;
 #endif
 
-	if (!func->old_addr || klp_is_module(obj))
-		ret = klp_find_object_symbol(obj->name, func->old_name,
-					     &func->old_addr);
+	if (func->old_addr)
+		ret = klp_verify_symbol(obj->name, func->old_name,
+				func->old_addr);
 	else
-		ret = klp_verify_vmlinux_symbol(func->old_name,
-						func->old_addr);
+		ret = klp_find_object_symbol(obj->name, func->old_name,
+				&func->old_addr);
 
 	return ret;
 }
@@ -285,10 +309,8 @@ static int klp_write_object_relocations(struct module *pmod,
 
 	for (reloc = obj->relocs; reloc->name; reloc++) {
 		if (!klp_is_module(obj)) {
-			ret = klp_verify_vmlinux_symbol(reloc->name,
+			ret = klp_verify_symbol(NULL, reloc->name,
 							reloc->val);
-			if (ret)
-				return ret;
 		} else {
 			/* module, reloc->val needs to be discovered */
 			if (reloc->external)
@@ -299,9 +321,9 @@ static int klp_write_object_relocations(struct module *pmod,
 				ret = klp_find_object_symbol(obj->mod->name,
 							     reloc->name,
 							     &reloc->val);
-			if (ret)
-				return ret;
 		}
+		if (ret)
+			return ret;
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
 					     reloc->val + reloc->addend);
 		if (ret) {
-- 
2.2.2

[PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 <minfei.huang@hotmail.com>
---
 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

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Petr Mladek]

On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> In order to restrict the patch, we can verify the provided function
> address and name match. Now we have can only verify the vmlinux function
> name and address.
> 
> Add a new function to verify extra module function name and address. The
> patch would not be patched, if the function name and address are not
> matched.

old_addr could be predefined only for vmlinux. It does not make sense
to define it for modules because they are loaded dynamically, each
time on a different addresses. It means that it does not make sense
to verify addresses from modules. They always need to be detected.

Best Regards,
Petr

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Petr Mladek]

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 <minfei.huang@hotmail.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/13/15 at 10:37P, Petr Mladek wrote:
> On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> > In order to restrict the patch, we can verify the provided function
> > address and name match. Now we have can only verify the vmlinux function
> > name and address.
> > 
> > Add a new function to verify extra module function name and address. The
> > patch would not be patched, if the function name and address are not
> > matched.
> 
> old_addr could be predefined only for vmlinux. It does not make sense
> to define it for modules because they are loaded dynamically, each
> time on a different addresses. It means that it does not make sense
> to verify addresses from modules. They always need to be detected.
> 

Please correct me if there is something wrong for below comment.

As commented in the doc that function address is optional, it is more
confortable during patching the patch, if function name and address are
provided.

For now we only use function name to detect the module function. It is
more accurate to detect the function using function name and address.

Maybe the function address being optional to be added is more accepted. 

Also what the patches's purpose is to support the situation that
function name is larger than 128.

I think the patches make sense, because we can not disallow the extra
module to be patch, which function name may be larger than 128.

Thanks
Minfei

> Best Regards,
> Petr

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 <minfei.huang@hotmail.com>
> > ---
> >  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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Petr Mladek]

On Mon 2015-04-13 17:11:19, Minfei Huang wrote:
> On 04/13/15 at 10:37P, Petr Mladek wrote:
> > On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> > > In order to restrict the patch, we can verify the provided function
> > > address and name match. Now we have can only verify the vmlinux function
> > > name and address.
> > > 
> > > Add a new function to verify extra module function name and address. The
> > > patch would not be patched, if the function name and address are not
> > > matched.
> > 
> > old_addr could be predefined only for vmlinux. It does not make sense
> > to define it for modules because they are loaded dynamically, each
> > time on a different addresses. It means that it does not make sense
> > to verify addresses from modules. They always need to be detected.
> > 
> 
> Please correct me if there is something wrong for below comment.
> 
> As commented in the doc that function address is optional, it is more
> confortable during patching the patch, if function name and address are
> provided.
> 
> For now we only use function name to detect the module function. It is
> more accurate to detect the function using function name and address.

I think that it is the other way. It is easier to create patch without
the addresses. It is enough for most patches.

The function address is needed only if there are more functions of
the same name. There are only few in vmlinux.

We currently does not allow to handle name conflicts inside a module.
But the chance is very small that there will be such a conflict.
Do you know about any module that could not be patched because
of this?


> Maybe the function address being optional to be added is more accepted. 
> 
> Also what the patches's purpose is to support the situation that
> function name is larger than 128.
>
> I think the patches make sense, because we can not disallow the extra
> module to be patch, which function name may be larger than 128.

Do you know about any such function name, please?

Best Regards,
Petr

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/13/15 at 11:41P, Petr Mladek wrote:
> On Mon 2015-04-13 17:11:19, Minfei Huang wrote:
> > On 04/13/15 at 10:37P, Petr Mladek wrote:
> > > On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> > > > In order to restrict the patch, we can verify the provided function
> > > > address and name match. Now we have can only verify the vmlinux function
> > > > name and address.
> > > > 
> > > > Add a new function to verify extra module function name and address. The
> > > > patch would not be patched, if the function name and address are not
> > > > matched.
> > > 
> > > old_addr could be predefined only for vmlinux. It does not make sense
> > > to define it for modules because they are loaded dynamically, each
> > > time on a different addresses. It means that it does not make sense
> > > to verify addresses from modules. They always need to be detected.
> > > 
> > 
> > Please correct me if there is something wrong for below comment.
> > 
> > As commented in the doc that function address is optional, it is more
> > confortable during patching the patch, if function name and address are
> > provided.
> > 
> > For now we only use function name to detect the module function. It is
> > more accurate to detect the function using function name and address.
> 
> I think that it is the other way. It is easier to create patch without
> the addresses. It is enough for most patches.
> 
> The function address is needed only if there are more functions of
> the same name. There are only few in vmlinux.
> 
> We currently does not allow to handle name conflicts inside a module.
> But the chance is very small that there will be such a conflict.
> Do you know about any module that could not be patched because
> of this?
> 

Yes, the function name exceeding to 128 is not happened, in general.
But I donot think it is the reason that livepatch donot support this
case.

Patched these patches, we can still use function name to detect function
for both vmlinux and extra module.

It is poisonless that we support to verify both function name and
address to detect function for extra module.

Thanks
Minfei

> 
> > Maybe the function address being optional to be added is more accepted. 
> > 
> > Also what the patches's purpose is to support the situation that
> > function name is larger than 128.
> >
> > I think the patches make sense, because we can not disallow the extra
> > module to be patch, which function name may be larger than 128.
> 
> Do you know about any such function name, please?
> 
> Best Regards,
> Petr

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Petr Mladek]

On Mon 2015-04-13 17:50:23, Minfei Huang wrote:
> On 04/13/15 at 11:41P, Petr Mladek wrote:
> > On Mon 2015-04-13 17:11:19, Minfei Huang wrote:
> > > On 04/13/15 at 10:37P, Petr Mladek wrote:
> > > > On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> > > > > In order to restrict the patch, we can verify the provided function
> > > > > address and name match. Now we have can only verify the vmlinux function
> > > > > name and address.
> > > > > 
> > > > > Add a new function to verify extra module function name and address. The
> > > > > patch would not be patched, if the function name and address are not
> > > > > matched.
> > > > 
> > > > old_addr could be predefined only for vmlinux. It does not make sense
> > > > to define it for modules because they are loaded dynamically, each
> > > > time on a different addresses. It means that it does not make sense
> > > > to verify addresses from modules. They always need to be detected.
> > > > 
> > > 
> > > Please correct me if there is something wrong for below comment.
> > > 
> > > As commented in the doc that function address is optional, it is more
> > > confortable during patching the patch, if function name and address are
> > > provided.
> > > 
> > > For now we only use function name to detect the module function. It is
> > > more accurate to detect the function using function name and address.
> > 
> > I think that it is the other way. It is easier to create patch without
> > the addresses. It is enough for most patches.
> > 
> > The function address is needed only if there are more functions of
> > the same name. There are only few in vmlinux.
> > 
> > We currently does not allow to handle name conflicts inside a module.
> > But the chance is very small that there will be such a conflict.
> > Do you know about any module that could not be patched because
> > of this?
> > 
> 
> Yes, the function name exceeding to 128 is not happened, in general.
> But I donot think it is the reason that livepatch donot support this
> case.
> 
> Patched these patches, we can still use function name to detect function
> for both vmlinux and extra module.
> 
> It is poisonless that we support to verify both function name and
> address to detect function for extra module.

Please, read my previous mails. We do not know old_addr for modules
at build time. Therefore we could solve this problem easily for
modules.

I think that the whole problem is rather theoretical and it is not
worth spending time on it.

Best Regards,
Petr

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/13/15 at 12:22P, Petr Mladek wrote:
> On Mon 2015-04-13 17:50:23, Minfei Huang wrote:
> > On 04/13/15 at 11:41P, Petr Mladek wrote:
> > > On Mon 2015-04-13 17:11:19, Minfei Huang wrote:
> > > > On 04/13/15 at 10:37P, Petr Mladek wrote:
> > > > > On Sun 2015-04-12 21:15:53, Minfei Huang wrote:
> > > > > > In order to restrict the patch, we can verify the provided function
> > > > > > address and name match. Now we have can only verify the vmlinux function
> > > > > > name and address.
> > > > > > 
> > > > > > Add a new function to verify extra module function name and address. The
> > > > > > patch would not be patched, if the function name and address are not
> > > > > > matched.
> > > > > 
> > > > > old_addr could be predefined only for vmlinux. It does not make sense
> > > > > to define it for modules because they are loaded dynamically, each
> > > > > time on a different addresses. It means that it does not make sense
> > > > > to verify addresses from modules. They always need to be detected.
> > > > > 
> > > > 
> > > > Please correct me if there is something wrong for below comment.
> > > > 
> > > > As commented in the doc that function address is optional, it is more
> > > > confortable during patching the patch, if function name and address are
> > > > provided.
> > > > 
> > > > For now we only use function name to detect the module function. It is
> > > > more accurate to detect the function using function name and address.
> > > 
> > > I think that it is the other way. It is easier to create patch without
> > > the addresses. It is enough for most patches.
> > > 
> > > The function address is needed only if there are more functions of
> > > the same name. There are only few in vmlinux.
> > > 
> > > We currently does not allow to handle name conflicts inside a module.
> > > But the chance is very small that there will be such a conflict.
> > > Do you know about any module that could not be patched because
> > > of this?
> > > 
> > 
> > Yes, the function name exceeding to 128 is not happened, in general.
> > But I donot think it is the reason that livepatch donot support this
> > case.
> > 
> > Patched these patches, we can still use function name to detect function
> > for both vmlinux and extra module.
> > 
> > It is poisonless that we support to verify both function name and
> > address to detect function for extra module.
> 
> Please, read my previous mails. We do not know old_addr for modules
> at build time. Therefore we could solve this problem easily for
> modules.
> 

Sorry, Do not get your point correctly.

For my patches, I think it is used by the persion which will compose the
patch individually, not for the manufactor. 

Yes, Verifying extra function address is more useless in general, due to
the changable address on different system.

IMO, we shall do our best to make livepatch more robust.

Thanks
Minfei

> I think that the whole problem is rather theoretical and it is not
> worth spending time on it.
> 
> Best Regards,
> Petr
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Josh Poimboeuf]

On Mon, Apr 13, 2015 at 06:37:10PM +0800, Minfei Huang wrote:
> For my patches, I think it is used by the persion which will compose the
> patch individually, not for the manufactor. 
> 
> Yes, Verifying extra function address is more useless in general, due to
> the changable address on different system.
> 
> IMO, we shall do our best to make livepatch more robust.

IIUC, to use this, you'd have to load the module first, manually look up
the module function's address, and _then_ build the patch for the
running system.  And the resulting patch wouldn't work on other systems.

Do you have concrete plans to use it this way?

Just trying to understand if this is needed for a real world usage
scenario.

-- 
Josh

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Josh Poimboeuf]

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

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/13/15 at 05:58P, Josh Poimboeuf wrote:
> On Mon, Apr 13, 2015 at 06:37:10PM +0800, Minfei Huang wrote:
> > For my patches, I think it is used by the persion which will compose the
> > patch individually, not for the manufactor. 
> > 
> > Yes, Verifying extra function address is more useless in general, due to
> > the changable address on different system.
> > 
> > IMO, we shall do our best to make livepatch more robust.
> 
> IIUC, to use this, you'd have to load the module first, manually look up
> the module function's address, and _then_ build the patch for the
> running system.  And the resulting patch wouldn't work on other systems.
> 
> Do you have concrete plans to use it this way?
> 
> Just trying to understand if this is needed for a real world usage
> scenario.

For some companies(like cloud computing company), they will compose
their own module to improve the performance.

Once there is some bug for the own module, they cannt restart to reload
the fixed-module. So it seems that livepatch is the best way to fix this
issue.

Before livepatch being integrated in kernel, we usually use ksplice to
patch the patch.

What the above scenario I met is in my previous work. 

For now, livepatch cannt patch the patch for extra module, once the
function name is larger than 127.

Thanks
Minfei

> 
> -- 
> Josh

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/14/15 at 08:17P, Minfei Huang wrote:
> On 04/13/15 at 05:58P, Josh Poimboeuf wrote:
> > On Mon, Apr 13, 2015 at 06:37:10PM +0800, Minfei Huang wrote:
> > > For my patches, I think it is used by the persion which will compose the
> > > patch individually, not for the manufactor. 
> > > 
> > > Yes, Verifying extra function address is more useless in general, due to
> > > the changable address on different system.
> > > 
> > > IMO, we shall do our best to make livepatch more robust.
> > 
> > IIUC, to use this, you'd have to load the module first, manually look up
> > the module function's address, and _then_ build the patch for the
> > running system.  And the resulting patch wouldn't work on other systems.
> > 
> > Do you have concrete plans to use it this way?
> > 
> > Just trying to understand if this is needed for a real world usage
> > scenario.
> 
> For some companies(like cloud computing company), they will compose
> their own module to improve the performance.
> 
> Once there is some bug for the own module, they cannt restart to reload
> the fixed-module. So it seems that livepatch is the best way to fix this
> issue.
> 
> Before livepatch being integrated in kernel, we usually use ksplice to
> patch the patch.
> 
> What the above scenario I met is in my previous work. 
> 
> For now, livepatch cannt patch the patch for extra module, once the
> function name is larger than 127.
> 

Also, Maybe there is some day, we can use script to detect the function
name and address in userspace, then generate the patch to patch the
defective kernel or extra module.

So the people who want to use livepatch never concern how to compose the
patch to patch the kernel or extra module by using livepatch. All they
will do is to provide a common patch which is different with the
original code.

Thanks
Minfei

> Thanks
> Minfei
> 
> > 
> > -- 
> > Josh

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Josh Poimboeuf]

On Tue, Apr 14, 2015 at 08:48:11AM +0800, Minfei Huang wrote:
> On 04/14/15 at 08:17P, Minfei Huang wrote:
> > On 04/13/15 at 05:58P, Josh Poimboeuf wrote:
> > > On Mon, Apr 13, 2015 at 06:37:10PM +0800, Minfei Huang wrote:
> > > > For my patches, I think it is used by the persion which will compose the
> > > > patch individually, not for the manufactor. 
> > > > 
> > > > Yes, Verifying extra function address is more useless in general, due to
> > > > the changable address on different system.
> > > > 
> > > > IMO, we shall do our best to make livepatch more robust.
> > > 
> > > IIUC, to use this, you'd have to load the module first, manually look up
> > > the module function's address, and _then_ build the patch for the
> > > running system.  And the resulting patch wouldn't work on other systems.
> > > 
> > > Do you have concrete plans to use it this way?
> > > 
> > > Just trying to understand if this is needed for a real world usage
> > > scenario.
> > 
> > For some companies(like cloud computing company), they will compose
> > their own module to improve the performance.
> > 
> > Once there is some bug for the own module, they cannt restart to reload
> > the fixed-module. So it seems that livepatch is the best way to fix this
> > issue.
> > 
> > Before livepatch being integrated in kernel, we usually use ksplice to
> > patch the patch.
> > 
> > What the above scenario I met is in my previous work. 
> > 
> > For now, livepatch cannt patch the patch for extra module, once the
> > function name is larger than 127.
> > 
> 
> Also, Maybe there is some day, we can use script to detect the function
> name and address in userspace, then generate the patch to patch the
> defective kernel or extra module.

I'd rather wait until we have a real world use case before adding
support for that.  Otherwise we end up bloating the code and have to
support a nebulous feature which nobody uses.

> So the people who want to use livepatch never concern how to compose the
> patch to patch the kernel or extra module by using livepatch. All they
> will do is to provide a common patch which is different with the
> original code.

We already have a kpatch tool named kpatch-build which does this.  It is
not yet upstreamed into Linux.  The key difference is that it creates
the patch at compile time rather than runtime.  The resulting patch
works for _all_ systems running the given version of kernel, rather than
only the current system.

-- 
Josh

Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module

[Minfei Huang]

On 04/13/15 at 11:05P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 08:48:11AM +0800, Minfei Huang wrote:
> > On 04/14/15 at 08:17P, Minfei Huang wrote:
> > > On 04/13/15 at 05:58P, Josh Poimboeuf wrote:
> > > > On Mon, Apr 13, 2015 at 06:37:10PM +0800, Minfei Huang wrote:
> > > > > For my patches, I think it is used by the persion which will compose the
> > > > > patch individually, not for the manufactor. 
> > > > > 
> > > > > Yes, Verifying extra function address is more useless in general, due to
> > > > > the changable address on different system.
> > > > > 
> > > > > IMO, we shall do our best to make livepatch more robust.
> > > > 
> > > > IIUC, to use this, you'd have to load the module first, manually look up
> > > > the module function's address, and _then_ build the patch for the
> > > > running system.  And the resulting patch wouldn't work on other systems.
> > > > 
> > > > Do you have concrete plans to use it this way?
> > > > 
> > > > Just trying to understand if this is needed for a real world usage
> > > > scenario.
> > > 
> > > For some companies(like cloud computing company), they will compose
> > > their own module to improve the performance.
> > > 
> > > Once there is some bug for the own module, they cannt restart to reload
> > > the fixed-module. So it seems that livepatch is the best way to fix this
> > > issue.
> > > 
> > > Before livepatch being integrated in kernel, we usually use ksplice to
> > > patch the patch.
> > > 
> > > What the above scenario I met is in my previous work. 
> > > 
> > > For now, livepatch cannt patch the patch for extra module, once the
> > > function name is larger than 127.
> > > 
> > 
> > Also, Maybe there is some day, we can use script to detect the function
> > name and address in userspace, then generate the patch to patch the
> > defective kernel or extra module.
> 
> I'd rather wait until we have a real world use case before adding
> support for that.  Otherwise we end up bloating the code and have to
> support a nebulous feature which nobody uses.
> 

Hi, Josh.

The above scenario is not fake to be suit for the patches. And it is
normal that end user composes patch to patch the kernel for extra module.
It is significative that livepatch support to patch extra module.

Livepatch is more important for the system which cannt reboot without
schedule.

> > So the people who want to use livepatch never concern how to compose the
> > patch to patch the kernel or extra module by using livepatch. All they
> > will do is to provide a common patch which is different with the
> > original code.
> 
> We already have a kpatch tool named kpatch-build which does this.  It is
> not yet upstreamed into Linux.  The key difference is that it creates
> the patch at compile time rather than runtime.  The resulting patch
> works for _all_ systems running the given version of kernel, rather than
> only the current system.
> 

Yes, Linda mentioned the kpatch on one of the meeting. But we cannot
only consider what we know, because the end user's environment is
complicated.

For my previous work, the extra module which uses to improve the
performance is running on CentOS6.3, CentOS6.5. For per fixing, we will
compose the patch on different kernel version(maybe the different
zstream kernel version).

Meanwhile, for the patches, I just want to add a new function that end
user can have chance to use function name and function address to match
the function for extra module, not only the function name. Also maybe
the specified patch is only for the currect system.

Thanks
Minfei

> -- 
> Josh

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Josh Poimboeuf]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Josh Poimboeuf]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Josh Poimboeuf]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Josh Poimboeuf]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Petr Mladek]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Petr Mladek]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 <minfei.huang@hotmail.com>
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 <minfei.huang@hotmail.com>
---
 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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Miroslav Benes]

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 <minfei.huang@hotmail.com>
> 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 <minfei.huang@hotmail.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Miroslav Benes]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Justin Keller]

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 <mbenes@suse.cz> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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 <mbenes@suse.cz> 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

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

Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Miroslav Benes]

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