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