* [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module [not found] <1428844554-4015-1-git-send-email-minfei.huang@hotmail.com> @ 2015-04-12 13:15 ` Minfei Huang 2015-04-13 8:37 ` Petr Mladek 2015-04-12 13:15 ` [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 Minfei Huang 1 sibling, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-12 13:15 UTC (permalink / raw) To: jpoimboe, sjenning, jkosina, vojtech Cc: live-patching, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-12 13:15 ` [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module Minfei Huang @ 2015-04-13 8:37 ` Petr Mladek 2015-04-13 9:11 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-13 8:37 UTC (permalink / raw) To: Minfei Huang Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 8:37 ` Petr Mladek @ 2015-04-13 9:11 ` Minfei Huang 2015-04-13 9:41 ` Petr Mladek 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-13 9:11 UTC (permalink / raw) To: Petr Mladek Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 9:11 ` Minfei Huang @ 2015-04-13 9:41 ` Petr Mladek 2015-04-13 9:50 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-13 9:41 UTC (permalink / raw) To: Minfei Huang Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 9:41 ` Petr Mladek @ 2015-04-13 9:50 ` Minfei Huang 2015-04-13 10:22 ` Petr Mladek 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-13 9:50 UTC (permalink / raw) To: Petr Mladek Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 9:50 ` Minfei Huang @ 2015-04-13 10:22 ` Petr Mladek 2015-04-13 10:37 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-13 10:22 UTC (permalink / raw) To: Minfei Huang Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 10:22 ` Petr Mladek @ 2015-04-13 10:37 ` Minfei Huang 2015-04-13 22:58 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-13 10:37 UTC (permalink / raw) To: Petr Mladek Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 10:37 ` Minfei Huang @ 2015-04-13 22:58 ` Josh Poimboeuf 2015-04-14 0:17 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-13 22:58 UTC (permalink / raw) To: Minfei Huang Cc: Petr Mladek, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-13 22:58 ` Josh Poimboeuf @ 2015-04-14 0:17 ` Minfei Huang 2015-04-14 0:48 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 0:17 UTC (permalink / raw) To: Josh Poimboeuf Cc: Petr Mladek, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-14 0:17 ` Minfei Huang @ 2015-04-14 0:48 ` Minfei Huang 2015-04-14 4:05 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 0:48 UTC (permalink / raw) To: Josh Poimboeuf Cc: Petr Mladek, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-14 0:48 ` Minfei Huang @ 2015-04-14 4:05 ` Josh Poimboeuf 2015-04-14 4:56 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-14 4:05 UTC (permalink / raw) To: Minfei Huang Cc: Petr Mladek, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module 2015-04-14 4:05 ` Josh Poimboeuf @ 2015-04-14 4:56 ` Minfei Huang 0 siblings, 0 replies; 37+ messages in thread From: Minfei Huang @ 2015-04-14 4:56 UTC (permalink / raw) To: Josh Poimboeuf Cc: Petr Mladek, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 [not found] <1428844554-4015-1-git-send-email-minfei.huang@hotmail.com> 2015-04-12 13:15 ` [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module Minfei Huang @ 2015-04-12 13:15 ` Minfei Huang 2015-04-13 8:44 ` Petr Mladek 2015-04-13 23:13 ` Josh Poimboeuf 1 sibling, 2 replies; 37+ messages in thread From: Minfei Huang @ 2015-04-12 13:15 UTC (permalink / raw) To: jpoimboe, sjenning, jkosina, vojtech Cc: live-patching, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-12 13:15 ` [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 Minfei Huang @ 2015-04-13 8:44 ` Petr Mladek 2015-04-13 9:16 ` Minfei Huang 2015-04-13 23:13 ` Josh Poimboeuf 1 sibling, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-13 8:44 UTC (permalink / raw) To: Minfei Huang Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-13 8:44 ` Petr Mladek @ 2015-04-13 9:16 ` Minfei Huang 0 siblings, 0 replies; 37+ messages in thread From: Minfei Huang @ 2015-04-13 9:16 UTC (permalink / raw) To: Petr Mladek Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-12 13:15 ` [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 Minfei Huang 2015-04-13 8:44 ` Petr Mladek @ 2015-04-13 23:13 ` Josh Poimboeuf 2015-04-14 0:26 ` Minfei Huang 1 sibling, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-13 23:13 UTC (permalink / raw) To: Minfei Huang; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-13 23:13 ` Josh Poimboeuf @ 2015-04-14 0:26 ` Minfei Huang 2015-04-14 4:57 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 0:26 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 0:26 ` Minfei Huang @ 2015-04-14 4:57 ` Josh Poimboeuf 2015-04-14 5:03 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-14 4:57 UTC (permalink / raw) To: Minfei Huang; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 4:57 ` Josh Poimboeuf @ 2015-04-14 5:03 ` Minfei Huang 2015-04-14 5:11 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 5:03 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 5:03 ` Minfei Huang @ 2015-04-14 5:11 ` Josh Poimboeuf 2015-04-14 5:29 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-14 5:11 UTC (permalink / raw) To: Minfei Huang; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 5:11 ` Josh Poimboeuf @ 2015-04-14 5:29 ` Minfei Huang 2015-04-14 5:32 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 5:29 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 5:29 ` Minfei Huang @ 2015-04-14 5:32 ` Josh Poimboeuf 2015-04-14 5:45 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-14 5:32 UTC (permalink / raw) To: Minfei Huang; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 5:32 ` Josh Poimboeuf @ 2015-04-14 5:45 ` Minfei Huang 2015-04-14 15:11 ` Josh Poimboeuf 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 5:45 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 5:45 ` Minfei Huang @ 2015-04-14 15:11 ` Josh Poimboeuf 2015-04-14 15:55 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Josh Poimboeuf @ 2015-04-14 15:11 UTC (permalink / raw) To: Minfei Huang; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 15:11 ` Josh Poimboeuf @ 2015-04-14 15:55 ` Minfei Huang 2015-04-14 16:27 ` Petr Mladek 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 15:55 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 15:55 ` Minfei Huang @ 2015-04-14 16:27 ` Petr Mladek 2015-04-14 17:01 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-14 16:27 UTC (permalink / raw) To: Minfei Huang Cc: Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 16:27 ` Petr Mladek @ 2015-04-14 17:01 ` Minfei Huang 2015-04-14 18:41 ` Petr Mladek 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-14 17:01 UTC (permalink / raw) To: Petr Mladek Cc: Minfei Huang, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 17:01 ` Minfei Huang @ 2015-04-14 18:41 ` Petr Mladek 2015-04-15 2:15 ` Minfei Huang 0 siblings, 1 reply; 37+ messages in thread From: Petr Mladek @ 2015-04-14 18:41 UTC (permalink / raw) To: Minfei Huang Cc: Minfei Huang, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-14 18:41 ` Petr Mladek @ 2015-04-15 2:15 ` Minfei Huang 2015-04-15 8:30 ` Miroslav Benes 0 siblings, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-15 2:15 UTC (permalink / raw) To: Petr Mladek Cc: Minfei Huang, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 2:15 ` Minfei Huang @ 2015-04-15 8:30 ` Miroslav Benes 2015-04-15 8:49 ` Minfei Huang 2015-04-15 10:35 ` Minfei Huang 0 siblings, 2 replies; 37+ messages in thread From: Miroslav Benes @ 2015-04-15 8:30 UTC (permalink / raw) To: Minfei Huang Cc: Petr Mladek, Minfei Huang, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 8:30 ` Miroslav Benes @ 2015-04-15 8:49 ` Minfei Huang 2015-04-15 10:35 ` Minfei Huang 1 sibling, 0 replies; 37+ messages in thread From: Minfei Huang @ 2015-04-15 8:49 UTC (permalink / raw) To: Miroslav Benes Cc: Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 8:30 ` Miroslav Benes 2015-04-15 8:49 ` Minfei Huang @ 2015-04-15 10:35 ` Minfei Huang 2015-04-15 11:58 ` Miroslav Benes 1 sibling, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-15 10:35 UTC (permalink / raw) To: Miroslav Benes Cc: Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 10:35 ` Minfei Huang @ 2015-04-15 11:58 ` Miroslav Benes 2015-04-15 16:24 ` Justin Keller 2015-04-26 13:05 ` Minfei Huang 0 siblings, 2 replies; 37+ messages in thread From: Miroslav Benes @ 2015-04-15 11:58 UTC (permalink / raw) To: Minfei Huang Cc: Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 11:58 ` Miroslav Benes @ 2015-04-15 16:24 ` Justin Keller 2015-04-16 2:10 ` Minfei Huang 2015-04-26 13:05 ` Minfei Huang 1 sibling, 1 reply; 37+ messages in thread From: Justin Keller @ 2015-04-15 16:24 UTC (permalink / raw) To: Miroslav Benes Cc: Minfei Huang, Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 16:24 ` Justin Keller @ 2015-04-16 2:10 ` Minfei Huang 0 siblings, 0 replies; 37+ messages in thread From: Minfei Huang @ 2015-04-16 2:10 UTC (permalink / raw) To: Justin Keller Cc: Miroslav Benes, Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-15 11:58 ` Miroslav Benes 2015-04-15 16:24 ` Justin Keller @ 2015-04-26 13:05 ` Minfei Huang 2015-04-27 8:41 ` Miroslav Benes 1 sibling, 1 reply; 37+ messages in thread From: Minfei Huang @ 2015-04-26 13:05 UTC (permalink / raw) To: Miroslav Benes Cc: Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 2015-04-26 13:05 ` Minfei Huang @ 2015-04-27 8:41 ` Miroslav Benes 0 siblings, 0 replies; 37+ messages in thread From: Miroslav Benes @ 2015-04-27 8:41 UTC (permalink / raw) To: Minfei Huang Cc: Minfei Huang, Petr Mladek, Josh Poimboeuf, sjenning, jkosina, vojtech, live-patching, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2015-04-27 8:41 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1428844554-4015-1-git-send-email-minfei.huang@hotmail.com> 2015-04-12 13:15 ` [PATCH 1/2] livepatch: Add a new function to verify the address and name match for extra module Minfei Huang 2015-04-13 8:37 ` Petr Mladek 2015-04-13 9:11 ` Minfei Huang 2015-04-13 9:41 ` Petr Mladek 2015-04-13 9:50 ` Minfei Huang 2015-04-13 10:22 ` Petr Mladek 2015-04-13 10:37 ` Minfei Huang 2015-04-13 22:58 ` Josh Poimboeuf 2015-04-14 0:17 ` Minfei Huang 2015-04-14 0:48 ` Minfei Huang 2015-04-14 4:05 ` Josh Poimboeuf 2015-04-14 4:56 ` Minfei Huang 2015-04-12 13:15 ` [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 Minfei Huang 2015-04-13 8:44 ` Petr Mladek 2015-04-13 9:16 ` Minfei Huang 2015-04-13 23:13 ` Josh Poimboeuf 2015-04-14 0:26 ` Minfei Huang 2015-04-14 4:57 ` Josh Poimboeuf 2015-04-14 5:03 ` Minfei Huang 2015-04-14 5:11 ` Josh Poimboeuf 2015-04-14 5:29 ` Minfei Huang 2015-04-14 5:32 ` Josh Poimboeuf 2015-04-14 5:45 ` Minfei Huang 2015-04-14 15:11 ` Josh Poimboeuf 2015-04-14 15:55 ` Minfei Huang 2015-04-14 16:27 ` Petr Mladek 2015-04-14 17:01 ` Minfei Huang 2015-04-14 18:41 ` Petr Mladek 2015-04-15 2:15 ` Minfei Huang 2015-04-15 8:30 ` Miroslav Benes 2015-04-15 8:49 ` Minfei Huang 2015-04-15 10:35 ` Minfei Huang 2015-04-15 11:58 ` Miroslav Benes 2015-04-15 16:24 ` Justin Keller 2015-04-16 2:10 ` Minfei Huang 2015-04-26 13:05 ` Minfei Huang 2015-04-27 8:41 ` Miroslav Benes
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.