Re: [PATCH 2/2] kernel: add support for live patching
On Sun, Nov 30, 2014 at 01:23:48PM +0100, Pavel Machek wrote:
> On Thu 2014-11-06 16:51:02, Jiri Slaby wrote:
> > On 11/06/2014, 03:39 PM, Seth Jennings wrote:
> > > This commit introduces code for the live patching core. It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
> >
> > Hi,
> >
> > nice! So we have something to start with. Brilliant!
> >
> > I have some comments below now. Yet, it obviously needs deeper review
> > which will take more time.
> >
> > > --- /dev/null
> > > +++ b/include/linux/livepatch.h
> > > @@ -0,0 +1,45 @@
> > > +#ifndef _LIVEPATCH_H_
> > > +#define _LIVEPATCH_H_
> >
> > This should follow the linux kernel naming: LINUX_LIVEPATCH_H
> >
> >
> > > +#include
> > > +
> > > +struct lp_func {
> >
> > I am not much happy with "lp" which effectively means parallel printer
> > support. What about lip?
>
> What about "patch_"?
Hey Pavel,
We are on v4 of this patchset:
https://lkml.org/lkml/2014/11/25/868
We ended up going with klp_ (kernel live patching).
Thanks,
Seth
>
> It is not so big subsystem that additional typing would matter much...
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu 2014-11-06 16:51:02, Jiri Slaby wrote:
> On 11/06/2014, 03:39 PM, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
>
> Hi,
>
> nice! So we have something to start with. Brilliant!
>
> I have some comments below now. Yet, it obviously needs deeper review
> which will take more time.
>
> > --- /dev/null
> > +++ b/include/linux/livepatch.h
> > @@ -0,0 +1,45 @@
> > +#ifndef _LIVEPATCH_H_
> > +#define _LIVEPATCH_H_
>
> This should follow the linux kernel naming: LINUX_LIVEPATCH_H
>
>
> > +#include
> > +
> > +struct lp_func {
>
> I am not much happy with "lp" which effectively means parallel printer
> support. What about lip?
What about "patch_"?
It is not so big subsystem that additional typing would matter much...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri 2014-11-14 14:30:30, Miroslav Benes wrote: > On Thu, 13 Nov 2014, Seth Jennings wrote: > > > On Thu, Nov 13, 2014 at 11:16:00AM +0100, Miroslav Benes wrote: > > > > > > Hi, > > > > > > thank you for the first version of the united live patching core. > > > > > > The patch below implements some of our review objections. Changes are > > > described in the commit log. It simplifies the hierarchy of data > > > structures, removes data duplication (lp_ and lpc_ structures) and > > > simplifies sysfs directory. > > > > > > I did not try to repair other stuff (races, function names, function > > > prefix, api symmetry etc.). It should serve as a demonstration of our > > > point of view. > > > > > > There are some problems with this. try_module_get and module_put may be > > > called several times for each kernel module where some function is > > > patched in. This should be fixed with module going notifier as suggested > > > by Petr. > > > > > > The modified core was tested with modified testing live patch originally > > > from Seth's github. It worked as expected. > > > > > > Please take a look at these changes, so we can discuss them in more > > > detail. > > > > Thanks Miroslav. > > > > The functional changes are a little hard to break out from the > > formatting changes like s/disable/unregister and s/lp_/lpc_/ or adding > > LPC_ prefix to the enum, most (all?) of which I have included for v2. > > > > A problem with getting rid of the object layer is that there are > > operations we do that are object-level operations. For example, > > module lookup and deferred module patching. Also, the dynamic > > relocations need to be associated with an object, not a patch, as not > > all relocations will be able to be applied at patch load time for > > patches that apply to modules that aren't loaded. I understand that you > > can walk the patch-level dynrela table and skip dynrela entries that > > don't match the target object, but why do that when you can cleanly > > express the relationship with a data structure hierarchy? > > > > One example is the call to is_object_loaded() (renamed and reworked in > > v2 btw) per function rather than per object. That is duplicate work and > > information that could be more cleanly expressed through an object > > layer. > > I understand your arguments as I had thought about this before. It is true > that some operations which are connected with the object level could be > duplicated in our approach. However the list of patched functions and > especially objects (vmlinux or modules) would be always relatively short. > Two-level hierarchy (functions and patches) is in my opinion more compact > and easier to maintain. I do not think that object-level outweighs this. > Let us think about it some more... > > > I also understand that sysfs/kobject stuff adds code length. However, > > the new "funcs" attribute is procfs style, not sysfs style. sysfs > > attribute should convey _one_ value. > > > > >From Documenation/filesystems/sysfs.txt: > > == > > Attributes should be ASCII text files, preferably with only one value > > per file. It is noted that it may not be efficient to contain only one > > value per file, so it is socially acceptable to express an array of > > values of the same type. > > > > Mixing types, expressing multiple lines of data, and doing fancy > > formatting of data is heavily frowned upon. Doing these things may get > > you publicly humiliated and your code rewritten without notice. > > == > > Ah, you are right. My mistake. Thank you. > > > Also the function list would have object ambiguity. If there was a > > patched function my_func() in both vmlinux and a module, it would just > > appear on the list twice. You can fix this by using the mod:func syntax > > like kallsyms, but it isn't as clean as expressing it in a hierarchy. > > Yes, using mod:func or check against module name (and not only function > name) would be necessary. Ambiguity would be also the problem in sysfs > directory tree without object level. But it is doable. > > > As far as the unification of the API structures with the internal > > structures I have two points. First is that, IMHO, we should assume that > > the structures coming from the user are const. In kpatch, for example, > > we pass through some structures that are not created in the code, but by > > the patch generation tool and stored in an ELF section (read-only). > > Additionally, I am really against exposing the internal fields. > > Commenting them as "internal" is just messy and we have to change the .h > > file every time when want to add a field for internal use. > > Changing the header file and thus API between different kernel releases > is not a problem in my opinion. First live patching module would be > created against specific kernel version (so the correct API is known). > Second we would like to add userspace tool for automatic patch generation > to
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, 13 Nov 2014, Seth Jennings wrote: > On Thu, Nov 13, 2014 at 11:16:00AM +0100, Miroslav Benes wrote: > > > > Hi, > > > > thank you for the first version of the united live patching core. > > > > The patch below implements some of our review objections. Changes are > > described in the commit log. It simplifies the hierarchy of data > > structures, removes data duplication (lp_ and lpc_ structures) and > > simplifies sysfs directory. > > > > I did not try to repair other stuff (races, function names, function > > prefix, api symmetry etc.). It should serve as a demonstration of our > > point of view. > > > > There are some problems with this. try_module_get and module_put may be > > called several times for each kernel module where some function is > > patched in. This should be fixed with module going notifier as suggested > > by Petr. > > > > The modified core was tested with modified testing live patch originally > > from Seth's github. It worked as expected. > > > > Please take a look at these changes, so we can discuss them in more > > detail. > > Thanks Miroslav. > > The functional changes are a little hard to break out from the > formatting changes like s/disable/unregister and s/lp_/lpc_/ or adding > LPC_ prefix to the enum, most (all?) of which I have included for v2. > > A problem with getting rid of the object layer is that there are > operations we do that are object-level operations. For example, > module lookup and deferred module patching. Also, the dynamic > relocations need to be associated with an object, not a patch, as not > all relocations will be able to be applied at patch load time for > patches that apply to modules that aren't loaded. I understand that you > can walk the patch-level dynrela table and skip dynrela entries that > don't match the target object, but why do that when you can cleanly > express the relationship with a data structure hierarchy? > > One example is the call to is_object_loaded() (renamed and reworked in > v2 btw) per function rather than per object. That is duplicate work and > information that could be more cleanly expressed through an object > layer. I understand your arguments as I had thought about this before. It is true that some operations which are connected with the object level could be duplicated in our approach. However the list of patched functions and especially objects (vmlinux or modules) would be always relatively short. Two-level hierarchy (functions and patches) is in my opinion more compact and easier to maintain. I do not think that object-level outweighs this. Let us think about it some more... > I also understand that sysfs/kobject stuff adds code length. However, > the new "funcs" attribute is procfs style, not sysfs style. sysfs > attribute should convey _one_ value. > > >From Documenation/filesystems/sysfs.txt: > == > Attributes should be ASCII text files, preferably with only one value > per file. It is noted that it may not be efficient to contain only one > value per file, so it is socially acceptable to express an array of > values of the same type. > > Mixing types, expressing multiple lines of data, and doing fancy > formatting of data is heavily frowned upon. Doing these things may get > you publicly humiliated and your code rewritten without notice. > == Ah, you are right. My mistake. Thank you. > Also the function list would have object ambiguity. If there was a > patched function my_func() in both vmlinux and a module, it would just > appear on the list twice. You can fix this by using the mod:func syntax > like kallsyms, but it isn't as clean as expressing it in a hierarchy. Yes, using mod:func or check against module name (and not only function name) would be necessary. Ambiguity would be also the problem in sysfs directory tree without object level. But it is doable. > As far as the unification of the API structures with the internal > structures I have two points. First is that, IMHO, we should assume that > the structures coming from the user are const. In kpatch, for example, > we pass through some structures that are not created in the code, but by > the patch generation tool and stored in an ELF section (read-only). > Additionally, I am really against exposing the internal fields. > Commenting them as "internal" is just messy and we have to change the .h > file every time when want to add a field for internal use. Changing the header file and thus API between different kernel releases is not a problem in my opinion. First live patching module would be created against specific kernel version (so the correct API is known). Second we would like to add userspace tool for automatic patch generation to upstream sometime in the future. API would be of "no importance" there as the situation in perf now (if I understand it correctly). > It seems that the primary purpose of this patch is to reduce the lines > of code. However, I think that the
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 13, 2014 at 11:16:00AM +0100, Miroslav Benes wrote: > > Hi, > > thank you for the first version of the united live patching core. > > The patch below implements some of our review objections. Changes are > described in the commit log. It simplifies the hierarchy of data > structures, removes data duplication (lp_ and lpc_ structures) and > simplifies sysfs directory. > > I did not try to repair other stuff (races, function names, function > prefix, api symmetry etc.). It should serve as a demonstration of our > point of view. > > There are some problems with this. try_module_get and module_put may be > called several times for each kernel module where some function is > patched in. This should be fixed with module going notifier as suggested > by Petr. > > The modified core was tested with modified testing live patch originally > from Seth's github. It worked as expected. > > Please take a look at these changes, so we can discuss them in more > detail. Thanks Miroslav. The functional changes are a little hard to break out from the formatting changes like s/disable/unregister and s/lp_/lpc_/ or adding LPC_ prefix to the enum, most (all?) of which I have included for v2. A problem with getting rid of the object layer is that there are operations we do that are object-level operations. For example, module lookup and deferred module patching. Also, the dynamic relocations need to be associated with an object, not a patch, as not all relocations will be able to be applied at patch load time for patches that apply to modules that aren't loaded. I understand that you can walk the patch-level dynrela table and skip dynrela entries that don't match the target object, but why do that when you can cleanly express the relationship with a data structure hierarchy? One example is the call to is_object_loaded() (renamed and reworked in v2 btw) per function rather than per object. That is duplicate work and information that could be more cleanly expressed through an object layer. I also understand that sysfs/kobject stuff adds code length. However, the new "funcs" attribute is procfs style, not sysfs style. sysfs attribute should convey _one_ value. >From Documenation/filesystems/sysfs.txt: == Attributes should be ASCII text files, preferably with only one value per file. It is noted that it may not be efficient to contain only one value per file, so it is socially acceptable to express an array of values of the same type. Mixing types, expressing multiple lines of data, and doing fancy formatting of data is heavily frowned upon. Doing these things may get you publicly humiliated and your code rewritten without notice. == Also the function list would have object ambiguity. If there was a patched function my_func() in both vmlinux and a module, it would just appear on the list twice. You can fix this by using the mod:func syntax like kallsyms, but it isn't as clean as expressing it in a hierarchy. As far as the unification of the API structures with the internal structures I have two points. First is that, IMHO, we should assume that the structures coming from the user are const. In kpatch, for example, we pass through some structures that are not created in the code, but by the patch generation tool and stored in an ELF section (read-only). Additionally, I am really against exposing the internal fields. Commenting them as "internal" is just messy and we have to change the .h file every time when want to add a field for internal use. It seems that the primary purpose of this patch is to reduce the lines of code. However, I think that the object layer of the data structure cleanly expresses the object<->function relationship and makes code like the deferred patching much more straightforward since you already have the functions/dynrelas organized by object. You don't have to do the nasty "if (strcmp(func->obj_name, objname)) continue;" business over the entire patch every time. Be advised, I have also done away with the new_addr/old_addr attributes for v2 and replaced the patched module ref'ing with a combination of a GOING notifier with lpc_mutex for protection. Thanks, Seth > > Best regards, > -- > Miroslav Benes > SUSE Labs > > > > From f659a18a630de27b47d375119d793e28ee50da04 Mon Sep 17 00:00:00 2001 > From: Miroslav Benes > Date: Thu, 13 Nov 2014 10:25:48 +0100 > Subject: [PATCH] lpc: simplification of structure and sysfs hierarchy > > Original code has several issues this patch tries to remove. > > First, there is only lpc_func structure for patched function and lpc_patch for > the patch as a whole. Therefore lpc_object structure as middle step of > hierarchy > is removed. Patched function is still associated with some object (vmlinux or > module) through obj_name. Dynrelas are now in lpc_patch structure and object > identifier (obj_name) is in the lpc_dynrela to preserve the connection. > > Second, sysfs structure is simplified. We d
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 13, 2014 at 11:16:00AM +0100, Miroslav Benes wrote: > > Hi, > > thank you for the first version of the united live patching core. > > The patch below implements some of our review objections. Changes are > described in the commit log. It simplifies the hierarchy of data > structures, removes data duplication (lp_ and lpc_ structures) and > simplifies sysfs directory. > > I did not try to repair other stuff (races, function names, function > prefix, api symmetry etc.). It should serve as a demonstration of our > point of view. > > There are some problems with this. try_module_get and module_put may be > called several times for each kernel module where some function is > patched in. This should be fixed with module going notifier as suggested > by Petr. > > The modified core was tested with modified testing live patch originally > from Seth's github. It worked as expected. > > Please take a look at these changes, so we can discuss them in more > detail. Hi Miroslav, Thanks for the code suggestions. This is a single patch with three major changes, which makes it hard to discuss each individual change on its own merits. Also, Seth has already made a lot of changes already based on previous comments, and is very close to having a v2 patch. Because this patch is so big, there are a lot of conflicts. Can you wait for the v2 patch set and then post your own patch set against it, with the patches split out so we can discuss them individually? -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
Hi,
thank you for the first version of the united live patching core.
The patch below implements some of our review objections. Changes are
described in the commit log. It simplifies the hierarchy of data
structures, removes data duplication (lp_ and lpc_ structures) and
simplifies sysfs directory.
I did not try to repair other stuff (races, function names, function
prefix, api symmetry etc.). It should serve as a demonstration of our
point of view.
There are some problems with this. try_module_get and module_put may be
called several times for each kernel module where some function is
patched in. This should be fixed with module going notifier as suggested
by Petr.
The modified core was tested with modified testing live patch originally
from Seth's github. It worked as expected.
Please take a look at these changes, so we can discuss them in more
detail.
Best regards,
--
Miroslav Benes
SUSE Labs
>From f659a18a630de27b47d375119d793e28ee50da04 Mon Sep 17 00:00:00 2001
From: Miroslav Benes
Date: Thu, 13 Nov 2014 10:25:48 +0100
Subject: [PATCH] lpc: simplification of structure and sysfs hierarchy
Original code has several issues this patch tries to remove.
First, there is only lpc_func structure for patched function and lpc_patch for
the patch as a whole. Therefore lpc_object structure as middle step of hierarchy
is removed. Patched function is still associated with some object (vmlinux or
module) through obj_name. Dynrelas are now in lpc_patch structure and object
identifier (obj_name) is in the lpc_dynrela to preserve the connection.
Second, sysfs structure is simplified. We do not need to propagate old_addr and
new_addr. So, there is subdirectory for each patch (patching module) which
includes original enabled attribute and new one funcs attribute which lists the
patched functions.
Third, data duplication (lp_ and lpc_ structures) is removed. lpc_ structures
are now in the header file and made available for the user. This allows us to
remove almost all the functions for structure allocation in the original code.
Signed-off-by: Miroslav Benes
---
include/linux/livepatch.h | 46 ++--
kernel/livepatch/core.c | 575 +-
2 files changed, 191 insertions(+), 430 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index c7a415b..db5ba00 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -2,10 +2,23 @@
#define _LIVEPATCH_H_
#include
+#include
-struct lp_func {
+enum lpc_state {
+ LPC_DISABLED,
+ LPC_ENABLED
+};
+
+struct lpc_func {
+ /* internal */
+ struct ftrace_ops fops;
+ enum lpc_state state;
+ struct module *mod; /* module associated with patched function */
+ unsigned long new_addr; /* replacement function in patch module */
+
+ /* external */
const char *old_name; /* function to be patched */
- void *new_func; /* replacement function in patch module */
+ void *new_func;
/*
* The old_addr field is optional and can be used to resolve
* duplicate symbol names in the vmlinux object. If this
@@ -15,31 +28,36 @@ struct lp_func {
* way to resolve the ambiguity.
*/
unsigned long old_addr;
+
+ const char *obj_name; /* "vmlinux" or module name */
};
-struct lp_dynrela {
+struct lpc_dynrela {
unsigned long dest;
unsigned long src;
unsigned long type;
const char *name;
+ const char *obj_name;
int addend;
int external;
};
-struct lp_object {
- const char *name; /* "vmlinux" or module name */
- struct lp_func *funcs;
- struct lp_dynrela *dynrelas;
-};
+struct lpc_patch {
+ /* internal */
+ struct list_head list;
+ struct kobject kobj;
+ enum lpc_state state;
-struct lp_patch {
+ /* external */
struct module *mod; /* module containing the patch */
- struct lp_object *objs;
+ struct lpc_dynrela *dynrelas;
+ struct lpc_func funcs[];
};
-int lp_register_patch(struct lp_patch *);
-int lp_unregister_patch(struct lp_patch *);
-int lp_enable_patch(struct lp_patch *);
-int lp_disable_patch(struct lp_patch *);
+
+extern int lpc_register_patch(struct lpc_patch *);
+extern int lpc_unregister_patch(struct lpc_patch *);
+extern int lpc_enable_patch(struct lpc_patch *);
+extern int lpc_disable_patch(struct lpc_patch *);
#endif /* _LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b32dbb5..feecc22 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -31,78 +31,32 @@
#include
+#define lpc_for_each_patch_func(p, pf) \
+for (pf = p->funcs; pf->old_name; pf++)
+
/*
* Core structures
/
-/*
- * lp_ structs vs lpc_ structs
- *
- * For each element (patch, object, func) in the live-patching code,
- *
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Tue, Nov 11, 2014 at 11:17:39PM +0100, Jiri Kosina wrote: > On Tue, 11 Nov 2014, Seth Jennings wrote: > > > It will be in v2 (hopefully out in the next couple of days). > > FWIW we are also working on a few patches on top of v1 to back some of the > proposals we've made during the first round of review, so maybe it might > make sense to wait with v2 a little bit more, so that it incorporates as > much v1 feedback as possible ... ? What proposals in particular? I've already made many of the changes that we agreed upon. Thanks, Seth > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Tue, 11 Nov 2014, Seth Jennings wrote: > It will be in v2 (hopefully out in the next couple of days). FWIW we are also working on a few patches on top of v1 to back some of the proposals we've made during the first round of review, so maybe it might make sense to wait with v2 a little bit more, so that it incorporates as much v1 feedback as possible ... ? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 07:40:11PM +0100, Petr Mladek wrote:
> On Fri 2014-11-07 12:07:11, Seth Jennings wrote:
> > On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
> > > On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
[...]
> > > > + up(&lpc_mutex);
> > > > + WARN("failed to apply patch '%s' to module '%s'\n",
> > > > + patch->mod->name, mod->name);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct notifier_block lp_module_nb = {
> > > > + .notifier_call = lp_module_notify,
> > > > + .priority = INT_MIN, /* called last */
> > >
> > > The handler for MODULE_STATE_COMMING would need have higger priority,
> > > if we want to cleanly unregister the ftrace handlers.
> >
> > Yes, we might need two handlers at different priorities if we decide to
> > go that direction: one for MODULE_STATE_GOING at high/max and one for
> > MODULE_STATE_COMING at low/min.
>
> kGraft has notifier only for the going state. The initialization is
> called directly from load_module() after ftrace_module_init()
> and complete_formation() before it is executed by parse_args().
>
> I need to investigate if the notifier is more elegant and safe or not.
I looked it up and having a COMING notifier with priority INT_MIN is
effectively the same as having a call between complete_formation() and
parse_args() since the notifiers are called as the last thing in
complete_formation().
I think I've found a clean way to avoid the ref taking on the patched
modules using only the notifier and lpc_mutex. It will be in v2
(hopefully out in the next couple of days).
Thanks,
Seth
>
> Best Regards,
> Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Mon, Nov 10, 2014 at 11:08:00AM +0100, Jiri Kosina wrote:
> On Thu, 6 Nov 2014, Seth Jennings wrote:
>
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > new file mode 100644
> > index 000..b32dbb5
> > --- /dev/null
> > +++ b/kernel/livepatch/core.c
>
> [ ... snip ... ]
>
> > +/
> > + * dynamic relocations (load-time linker)
> > + /
> > +
> > +/*
> > + * external symbols are located outside the parent object (where the parent
> > + * object is either vmlinux or the kmod being patched).
> > + */
> > +static int lpc_find_external_symbol(struct module *pmod, const char *name,
> > + unsigned long *addr)
> > +{
> > + const struct kernel_symbol *sym;
> > +
> > + /* first, check if it's an exported symbol */
> > + preempt_disable();
> > + sym = find_symbol(name, NULL, NULL, true, true);
> > + preempt_enable();
> > + if (sym) {
> > + *addr = sym->value;
> > + return 0;
> > + }
> > +
> > + /* otherwise check if it's in another .o within the patch module */
> > + return lpc_find_symbol(pmod->name, name, addr);
> > +}
> > +
> > +static int lpc_write_object_relocations(struct module *pmod,
> > + struct lpc_object *obj)
> > +{
> > + int ret, size, readonly = 0, numpages;
> > + struct lp_dynrela *dynrela;
> > + u64 loc, val;
> > + unsigned long core = (unsigned long)pmod->module_core;
> > + unsigned long core_ro_size = pmod->core_ro_size;
> > + unsigned long core_size = pmod->core_size;
> > +
> > + for (dynrela = obj->dynrelas; dynrela->name; dynrela++) {
> > + if (!strcmp(obj->name, "vmlinux")) {
> > + ret = lpc_verify_vmlinux_symbol(dynrela->name,
> > + dynrela->src);
> > + if (ret)
> > + return ret;
> > + } else {
> > + /* module, dynrela->src needs to be discovered */
> > + if (dynrela->external)
> > + ret = lpc_find_external_symbol(pmod,
> > + dynrela->name,
> > + &dynrela->src);
> > + else
> > + ret = lpc_find_symbol(obj->mod->name,
> > + dynrela->name,
> > + &dynrela->src);
> > + if (ret)
> > + return -EINVAL;
> > + }
> > +
> > + switch (dynrela->type) {
> > + case R_X86_64_NONE:
> > + continue;
> > + case R_X86_64_PC32:
> > + loc = dynrela->dest;
> > + val = (u32)(dynrela->src + dynrela->addend -
> > + dynrela->dest);
> > + size = 4;
> > + break;
> > + case R_X86_64_32S:
> > + loc = dynrela->dest;
> > + val = (s32)dynrela->src + dynrela->addend;
> > + size = 4;
> > + break;
> > + case R_X86_64_64:
> > + loc = dynrela->dest;
> > + val = dynrela->src;
> > + size = 8;
> > + break;
>
> This is x86-specific, so it definitely needs to go to arch/x86. The only
> hard precondition for arch to support live patching is ftrace with regs
> saving (we are currently in parallel working on extending the set of
> architectures that support this), so we shouldn't introduce any x86-isms
> into the generic code.
>
> It seems to me that what basically needs to be done is to teach
> apply_relocate_add() about this kind of relocations and apply them as
> needed.
Agreed.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, 6 Nov 2014, Seth Jennings wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> new file mode 100644
> index 000..b32dbb5
> --- /dev/null
> +++ b/kernel/livepatch/core.c
[ ... snip ... ]
> +/
> + * dynamic relocations (load-time linker)
> + /
> +
> +/*
> + * external symbols are located outside the parent object (where the parent
> + * object is either vmlinux or the kmod being patched).
> + */
> +static int lpc_find_external_symbol(struct module *pmod, const char *name,
> + unsigned long *addr)
> +{
> + const struct kernel_symbol *sym;
> +
> + /* first, check if it's an exported symbol */
> + preempt_disable();
> + sym = find_symbol(name, NULL, NULL, true, true);
> + preempt_enable();
> + if (sym) {
> + *addr = sym->value;
> + return 0;
> + }
> +
> + /* otherwise check if it's in another .o within the patch module */
> + return lpc_find_symbol(pmod->name, name, addr);
> +}
> +
> +static int lpc_write_object_relocations(struct module *pmod,
> + struct lpc_object *obj)
> +{
> + int ret, size, readonly = 0, numpages;
> + struct lp_dynrela *dynrela;
> + u64 loc, val;
> + unsigned long core = (unsigned long)pmod->module_core;
> + unsigned long core_ro_size = pmod->core_ro_size;
> + unsigned long core_size = pmod->core_size;
> +
> + for (dynrela = obj->dynrelas; dynrela->name; dynrela++) {
> + if (!strcmp(obj->name, "vmlinux")) {
> + ret = lpc_verify_vmlinux_symbol(dynrela->name,
> + dynrela->src);
> + if (ret)
> + return ret;
> + } else {
> + /* module, dynrela->src needs to be discovered */
> + if (dynrela->external)
> + ret = lpc_find_external_symbol(pmod,
> +dynrela->name,
> +&dynrela->src);
> + else
> + ret = lpc_find_symbol(obj->mod->name,
> + dynrela->name,
> + &dynrela->src);
> + if (ret)
> + return -EINVAL;
> + }
> +
> + switch (dynrela->type) {
> + case R_X86_64_NONE:
> + continue;
> + case R_X86_64_PC32:
> + loc = dynrela->dest;
> + val = (u32)(dynrela->src + dynrela->addend -
> + dynrela->dest);
> + size = 4;
> + break;
> + case R_X86_64_32S:
> + loc = dynrela->dest;
> + val = (s32)dynrela->src + dynrela->addend;
> + size = 4;
> + break;
> + case R_X86_64_64:
> + loc = dynrela->dest;
> + val = dynrela->src;
> + size = 8;
> + break;
This is x86-specific, so it definitely needs to go to arch/x86. The only
hard precondition for arch to support live patching is ftrace with regs
saving (we are currently in parallel working on extending the set of
architectures that support this), so we shouldn't introduce any x86-isms
into the generic code.
It seems to me that what basically needs to be done is to teach
apply_relocate_add() about this kind of relocations and apply them as
needed.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: more patches for the same func: was Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 06:39:03PM +0100, Petr Mladek wrote:
> On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
> >
> > It represents the greatest common functionality set between kpatch and
> > kgraft and can accept patches built using either method.
> >
> > This first version does not implement any consistency mechanism that
> > ensures that old and new code do not run together. In practice, ~90% of
> > CVEs are safe to apply in this way, since they simply add a conditional
> > check. However, any function change that can not execute safely with
> > the old version of the function can _not_ be safely applied in this
> > version.
>
> [...]
>
> > +static int lpc_enable_func(struct lpc_func *func)
> > +{
> > + int ret;
> > +
> > + BUG_ON(!func->old_addr);
> > + BUG_ON(func->state != DISABLED);
> > + ret = ftrace_set_filter_ip(&func->fops, func->old_addr, 0, 0);
> > + if (ret) {
> > + pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> > + func->old_name, ret);
> > + return ret;
> > + }
> > + ret = register_ftrace_function(&func->fops);
> > + if (ret) {
> > + pr_err("failed to register ftrace handler for function '%s'
> > (%d)\n",
> > + func->old_name, ret);
> > + ftrace_set_filter_ip(&func->fops, func->old_addr, 1, 0);
> > + } else
> > + func->state = ENABLED;
> > +
> > + return ret;
> > +}
> > +
>
> [...]
>
> > +/* caller must ensure that obj->mod is set if object is a module */
> > +static int lpc_enable_object(struct module *pmod, struct lpc_object *obj)
> > +{
> > + struct lpc_func *func;
> > + int ret;
> > +
> > + if (obj->mod && !try_module_get(obj->mod))
> > + return -ENODEV;
> > +
> > + if (obj->dynrelas) {
> > + ret = lpc_write_object_relocations(pmod, obj);
> > + if (ret)
> > + goto unregister;
> > + }
> > + list_for_each_entry(func, &obj->funcs, list) {
> > + ret = lpc_find_verify_func_addr(func, obj->name);
> > + if (ret)
> > + goto unregister;
> > +
> > + ret = lpc_enable_func(func);
> > + if (ret)
> > + goto unregister;
> > + }
> > + obj->state = ENABLED;
> > +
> > + return 0;
> > +unregister:
> > + WARN_ON(lpc_unregister_object(obj));
> > + return ret;
> > +}
> > +
> > +/**
> > + * enable/disable
> > + **/
> > +
> > +/* must be called with lpc_mutex held */
> > +static struct lpc_patch *lpc_find_patch(struct lp_patch *userpatch)
> > +{
> > + struct lpc_patch *patch;
> > +
> > + list_for_each_entry(patch, &lpc_patches, list)
> > + if (patch->userpatch == userpatch)
> > + return patch;
> > +
> > + return NULL;
> > +}
>
> [...]
>
> > +
> > +/* must be called with lpc_mutex held */
> > +static int lpc_enable_patch(struct lpc_patch *patch)
> > +{
> > + struct lpc_object *obj;
> > + int ret;
> > +
> > + BUG_ON(patch->state != DISABLED);
> > +
> > + pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> > + add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> > +
> > + pr_notice("enabling patch '%s'\n", patch->mod->name);
> > +
> > + list_for_each_entry(obj, &patch->objs, list) {
> > + if (!is_object_loaded(obj))
> > + continue;
> > + ret = lpc_enable_object(patch->mod, obj);
> > + if (ret)
> > + goto unregister;
> > + }
> > + patch->state = ENABLED;
> > + return 0;
> > +
> > +unregister:
> > + WARN_ON(lpc_disable_patch(patch));
> > + return ret;
> > +}
> > +
> > +int lp_enable_patch(struct lp_patch *userpatch)
> > +{
> > + struct lpc_patch *patch;
> > + int ret;
> > +
> > + down(&lpc_mutex);
> > + patch = lpc_find_patch(userpatch);
> > + if (!patch) {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > + ret = lpc_enable_patch(patch);
> > +out:
> > + up(&lpc_mutex);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(lp_enable_patch);
>
> AFAIK, this does not handle correctly the situation when there
> are more patches for the same symbol. IMHO, the first registered
> ftrace function wins. It means that later patches are ignored.
Yeah, good point. With kpatch we had a single ftrace_ops which was
shared for all patched functions, so we didn't have this problem. From
the ftrace handler, we would just get the most recent version of the
function from our hash table.
With livepatch we decided to change to the model of one ftrace ops per
patched function. Which I think is probably a good idea, but it's hard
to say for sure without knowing our consistency model yet.
>
> In kGraft, we detect this situation and do the following:
>
>
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 07:21:03PM +0100, Petr Mladek wrote: > On Thu 2014-11-06 10:57:48, Seth Jennings wrote: > > On Thu, Nov 06, 2014 at 04:51:02PM +0100, Jiri Slaby wrote: > > > On 11/06/2014, 03:39 PM, Seth Jennings wrote: > > > > +/* > > > > + * Core structures > > > > + / > > > > + > > > > +/* > > > > + * lp_ structs vs lpc_ structs > > > > + * > > > > + * For each element (patch, object, func) in the live-patching code, > > > > + * there are two types with two different prefixes: lp_ and lpc_. > > > > + * > > > > + * Structures used by the live-patch modules to register with this > > > > core module > > > > + * are prefixed with lp_ (live patching). These structures are part > > > > of the > > > > + * registration API and are defined in livepatch.h. The structures > > > > used > > > > + * internally by this core module are prefixed with lpc_ (live > > > > patching core). > > > > + */ > > > > > > I am not sure if the separation and the allocations/kobj handling are > > > worth it. It makes the code really less understandable. Can we have just > > > struct lip_function (don't unnecessarily abbreviate), lip_objectfile > > > (object is too generic, like Java object) and lip_patch containing all > > > the needed information? It would clean up the code a lot. (Yes, we would > > > have profited from c++ here.) > > > > I looked at doing this and this is actually what we did in kpatch. We > > made one structure that had "private" members that the user wasn't > > suppose to access that were only used in the core. This was messy > > though. Every time you wanted to add a "private" field to the struct so > > the core could do something new, you were changing the API to the patch > > modules as well. While copying the data into an internal structure does > > add code and opportunity for errors, that functionality is localized > > into functions that are specifically tasked with taking care of that. > > So the risk is minimized and we gain flexibility within the core and > > more self-documenting API structures. > > I am not sure if the modified API is really such a big limit. The > modules initialize the needed members using ".member = value". > Also we do not need to take care of API/ABI backward compatibility because > there is very strict dependency between patches and the patched > kernel. Our patch module generation tool (kpatch-build) relies on the API as well, so we should try to keep the API as stable as possible. At least until we can put kpatch-build (or something like it) into the kernel tree. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 11:40:38AM -0800, Andy Lutomirski wrote: > On 11/06/2014 06:39 AM, Seth Jennings wrote: > > This commit introduces code for the live patching core. It implements > > an ftrace-based mechanism and kernel interface for doing live patching > > of kernel and kernel module functions. > > > > It represents the greatest common functionality set between kpatch and > > kgraft and can accept patches built using either method. > > > > This first version does not implement any consistency mechanism that > > ensures that old and new code do not run together. In practice, ~90% of > > CVEs are safe to apply in this way, since they simply add a conditional > > check. However, any function change that can not execute safely with > > the old version of the function can _not_ be safely applied in this > > version. > > > > [...] > > > +/ > > + * Sysfs Interface > > + ***/ > > +/* > > + * /sys/kernel/livepatch > > + * /sys/kernel/livepatch/ > > + * /sys/kernel/livepatch//enabled > > + * /sys/kernel/livepatch// > > + * /sys/kernel/livepatch/// > > + * /sys/kernel/livepatchnew_addr > > + * /sys/kernel/livepatchold_addr > > + */ > > Letting anyone read new_addr and old_addr is a kASLR leak, and I would > argue that showing this information to non-root at all is probably a bad > idea. Also worth noting that this live patching implementation currently doesn't support kASLR, as there is a method for the patch module to supply the old_addr, determined at generation time by pulling from vmlinux/System.map/etc, for a particular function to resolve symbol ambiguity in a kallsyms lookup. Obviously, this old_addr would be wrong for a kernel using kASLR. Thanks, Seth > > Can you make new_addr and old_addr have mode 0600 and > /sys/kernel/livepatch itself have mode 0500? For the latter, an admin > who wants unprivileged users to be able to see it can easily chmod it. > > --Andy > -- > To unsubscribe from this list: send the line "unsubscribe live-patching" in > the body of a message to majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 11:40:38AM -0800, Andy Lutomirski wrote: > On 11/06/2014 06:39 AM, Seth Jennings wrote: > > This commit introduces code for the live patching core. It implements > > an ftrace-based mechanism and kernel interface for doing live patching > > of kernel and kernel module functions. > > > > It represents the greatest common functionality set between kpatch and > > kgraft and can accept patches built using either method. > > > > This first version does not implement any consistency mechanism that > > ensures that old and new code do not run together. In practice, ~90% of > > CVEs are safe to apply in this way, since they simply add a conditional > > check. However, any function change that can not execute safely with > > the old version of the function can _not_ be safely applied in this > > version. > > > > [...] > > > +/ > > + * Sysfs Interface > > + ***/ > > +/* > > + * /sys/kernel/livepatch > > + * /sys/kernel/livepatch/ > > + * /sys/kernel/livepatch//enabled > > + * /sys/kernel/livepatch// > > + * /sys/kernel/livepatch/// > > + * /sys/kernel/livepatchnew_addr > > + * /sys/kernel/livepatchold_addr > > + */ > > Letting anyone read new_addr and old_addr is a kASLR leak, and I would > argue that showing this information to non-root at all is probably a bad > idea. > > Can you make new_addr and old_addr have mode 0600 and > /sys/kernel/livepatch itself have mode 0500? For the latter, an admin > who wants unprivileged users to be able to see it can easily chmod it. Good call. Will do. Thanks, Seth > > --Andy > -- > To unsubscribe from this list: send the line "unsubscribe live-patching" in > the body of a message to majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On 11/06/2014 06:39 AM, Seth Jennings wrote: > This commit introduces code for the live patching core. It implements > an ftrace-based mechanism and kernel interface for doing live patching > of kernel and kernel module functions. > > It represents the greatest common functionality set between kpatch and > kgraft and can accept patches built using either method. > > This first version does not implement any consistency mechanism that > ensures that old and new code do not run together. In practice, ~90% of > CVEs are safe to apply in this way, since they simply add a conditional > check. However, any function change that can not execute safely with > the old version of the function can _not_ be safely applied in this > version. > [...] > +/ > + * Sysfs Interface > + ***/ > +/* > + * /sys/kernel/livepatch > + * /sys/kernel/livepatch/ > + * /sys/kernel/livepatch//enabled > + * /sys/kernel/livepatch// > + * /sys/kernel/livepatch/// > + * /sys/kernel/livepatchnew_addr > + * /sys/kernel/livepatchold_addr > + */ Letting anyone read new_addr and old_addr is a kASLR leak, and I would argue that showing this information to non-root at all is probably a bad idea. Can you make new_addr and old_addr have mode 0600 and /sys/kernel/livepatch itself have mode 0500? For the latter, an admin who wants unprivileged users to be able to see it can easily chmod it. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 07:40:11PM +0100, Petr Mladek wrote:
> On Fri 2014-11-07 12:07:11, Seth Jennings wrote:
> > On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
> > > On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> > > > This commit introduces code for the live patching core. It implements
> > > > an ftrace-based mechanism and kernel interface for doing live patching
> > > > of kernel and kernel module functions.
> > > >
> > > > It represents the greatest common functionality set between kpatch and
> > > > kgraft and can accept patches built using either method.
> > > >
> > > > This first version does not implement any consistency mechanism that
> > > > ensures that old and new code do not run together. In practice, ~90% of
> > > > CVEs are safe to apply in this way, since they simply add a conditional
> > > > check. However, any function change that can not execute safely with
> > > > the old version of the function can _not_ be safely applied in this
> > > > version.
> > >
> > > [...]
> > >
> > > > +/**
> > > > + * module notifier
> > > > + */
> > > > +
> > > > +static int lp_module_notify(struct notifier_block *nb, unsigned long
> > > > action,
> > > > + void *data)
> > > > +{
> > > > + struct module *mod = data;
> > > > + struct lpc_patch *patch;
> > > > + struct lpc_object *obj;
> > > > + int ret = 0;
> > > > +
> > > > + if (action != MODULE_STATE_COMING)
> > > > + return 0;
> > >
> > > IMHO, we should handle also MODULE_STATE_GOING. We should unregister
> > > the ftrace handlers and update the state of the affected objects
> > > (ENABLED -> DISABLED)
> >
> > The mechanism we use to avoid this right now is taking a reference on
> > patched module. We only release that reference after the patch is
> > disabled, which unregisters all the patched functions from ftrace.
>
> I see. This was actually another thing that I noticed and wanted to
> investigate :-) I think that we should not force users to disable
> the entire patch if they want to remove some module.
I agree that would be better.
>
>
> > However, your comment reminded me of an idea I had to use
> > MODULE_STATE_GOING and let the lpc_mutex protect against races. I think
> > it could be cleaner, but I haven't fleshed the idea out fully.
>
> AFAIK, the going module is not longer used when the notifier is
> called. Therefore we could remove the patch fast way even when
> patching would require the slow path otherwise.
Yes (Josh just brought this to my attention) is that the notifiers are
call with GOING _after_ the module's exit function is called.
Thanks,
Seth
>
>
> > >
> > > > + down(&lpc_mutex);
> > > > +
> > > > + list_for_each_entry(patch, &lpc_patches, list) {
> > > > + if (patch->state == DISABLED)
> > > > + continue;
> > > > + list_for_each_entry(obj, &patch->objs, list) {
> > > > + if (strcmp(obj->name, mod->name))
> > > > + continue;
> > > > + pr_notice("load of module '%s' detected,
> > > > applying patch '%s'\n",
> > > > + mod->name, patch->mod->name);
> > > > + obj->mod = mod;
> > > > + ret = lpc_enable_object(patch->mod, obj);
> > > > + if (ret)
> > > > + goto out;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + up(&lpc_mutex);
> > > > + return 0;
> > > > +out:
> > >
> > > I would name this err_our or so to make it clear that it is used when
> > > something fails.
> >
> > Just "err" good?
>
> Fine with me.
>
> > > > + up(&lpc_mutex);
> > > > + WARN("failed to apply patch '%s' to module '%s'\n",
> > > > + patch->mod->name, mod->name);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct notifier_block lp_module_nb = {
> > > > + .notifier_call = lp_module_notify,
> > > > + .priority = INT_MIN, /* called last */
> > >
> > > The handler for MODULE_STATE_COMMING would need have higger priority,
> > > if we want to cleanly unregister the ftrace handlers.
> >
> > Yes, we might need two handlers at different priorities if we decide to
> > go that direction: one for MODULE_STATE_GOING at high/max and one for
> > MODULE_STATE_COMING at low/min.
>
> kGraft has notifier only for the going state. The initialization is
> called directly from load_module() after ftrace_module_init()
> and complete_formation() before it is executed by parse_args().
>
> I need to investigate if the notifier is more elegant and safe or not.
>
> Best Regards,
> Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info a
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Fri 2014-11-07 12:07:11, Seth Jennings wrote:
> On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
> > On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> > > This commit introduces code for the live patching core. It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
> > >
> > > It represents the greatest common functionality set between kpatch and
> > > kgraft and can accept patches built using either method.
> > >
> > > This first version does not implement any consistency mechanism that
> > > ensures that old and new code do not run together. In practice, ~90% of
> > > CVEs are safe to apply in this way, since they simply add a conditional
> > > check. However, any function change that can not execute safely with
> > > the old version of the function can _not_ be safely applied in this
> > > version.
> >
> > [...]
> >
> > > +/**
> > > + * module notifier
> > > + */
> > > +
> > > +static int lp_module_notify(struct notifier_block *nb, unsigned long
> > > action,
> > > + void *data)
> > > +{
> > > + struct module *mod = data;
> > > + struct lpc_patch *patch;
> > > + struct lpc_object *obj;
> > > + int ret = 0;
> > > +
> > > + if (action != MODULE_STATE_COMING)
> > > + return 0;
> >
> > IMHO, we should handle also MODULE_STATE_GOING. We should unregister
> > the ftrace handlers and update the state of the affected objects
> > (ENABLED -> DISABLED)
>
> The mechanism we use to avoid this right now is taking a reference on
> patched module. We only release that reference after the patch is
> disabled, which unregisters all the patched functions from ftrace.
I see. This was actually another thing that I noticed and wanted to
investigate :-) I think that we should not force users to disable
the entire patch if they want to remove some module.
> However, your comment reminded me of an idea I had to use
> MODULE_STATE_GOING and let the lpc_mutex protect against races. I think
> it could be cleaner, but I haven't fleshed the idea out fully.
AFAIK, the going module is not longer used when the notifier is
called. Therefore we could remove the patch fast way even when
patching would require the slow path otherwise.
> >
> > > + down(&lpc_mutex);
> > > +
> > > + list_for_each_entry(patch, &lpc_patches, list) {
> > > + if (patch->state == DISABLED)
> > > + continue;
> > > + list_for_each_entry(obj, &patch->objs, list) {
> > > + if (strcmp(obj->name, mod->name))
> > > + continue;
> > > + pr_notice("load of module '%s' detected, applying patch
> > > '%s'\n",
> > > + mod->name, patch->mod->name);
> > > + obj->mod = mod;
> > > + ret = lpc_enable_object(patch->mod, obj);
> > > + if (ret)
> > > + goto out;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + up(&lpc_mutex);
> > > + return 0;
> > > +out:
> >
> > I would name this err_our or so to make it clear that it is used when
> > something fails.
>
> Just "err" good?
Fine with me.
> > > + up(&lpc_mutex);
> > > + WARN("failed to apply patch '%s' to module '%s'\n",
> > > + patch->mod->name, mod->name);
> > > + return 0;
> > > +}
> > > +
> > > +static struct notifier_block lp_module_nb = {
> > > + .notifier_call = lp_module_notify,
> > > + .priority = INT_MIN, /* called last */
> >
> > The handler for MODULE_STATE_COMMING would need have higger priority,
> > if we want to cleanly unregister the ftrace handlers.
>
> Yes, we might need two handlers at different priorities if we decide to
> go that direction: one for MODULE_STATE_GOING at high/max and one for
> MODULE_STATE_COMING at low/min.
kGraft has notifier only for the going state. The initialization is
called directly from load_module() after ftrace_module_init()
and complete_formation() before it is executed by parse_args().
I need to investigate if the notifier is more elegant and safe or not.
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu 2014-11-06 10:57:48, Seth Jennings wrote: > On Thu, Nov 06, 2014 at 04:51:02PM +0100, Jiri Slaby wrote: > > On 11/06/2014, 03:39 PM, Seth Jennings wrote: > > > +/* > > > + * Core structures > > > + / > > > + > > > +/* > > > + * lp_ structs vs lpc_ structs > > > + * > > > + * For each element (patch, object, func) in the live-patching code, > > > + * there are two types with two different prefixes: lp_ and lpc_. > > > + * > > > + * Structures used by the live-patch modules to register with this core > > > module > > > + * are prefixed with lp_ (live patching). These structures are part of > > > the > > > + * registration API and are defined in livepatch.h. The structures used > > > + * internally by this core module are prefixed with lpc_ (live patching > > > core). > > > + */ > > > > I am not sure if the separation and the allocations/kobj handling are > > worth it. It makes the code really less understandable. Can we have just > > struct lip_function (don't unnecessarily abbreviate), lip_objectfile > > (object is too generic, like Java object) and lip_patch containing all > > the needed information? It would clean up the code a lot. (Yes, we would > > have profited from c++ here.) > > I looked at doing this and this is actually what we did in kpatch. We > made one structure that had "private" members that the user wasn't > suppose to access that were only used in the core. This was messy > though. Every time you wanted to add a "private" field to the struct so > the core could do something new, you were changing the API to the patch > modules as well. While copying the data into an internal structure does > add code and opportunity for errors, that functionality is localized > into functions that are specifically tasked with taking care of that. > So the risk is minimized and we gain flexibility within the core and > more self-documenting API structures. I am not sure if the modified API is really such a big limit. The modules initialize the needed members using ".member = value". Also we do not need to take care of API/ABI backward compatibility because there is very strict dependency between patches and the patched kernel. Well, I have to think more about it. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
> On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
> >
> > It represents the greatest common functionality set between kpatch and
> > kgraft and can accept patches built using either method.
> >
> > This first version does not implement any consistency mechanism that
> > ensures that old and new code do not run together. In practice, ~90% of
> > CVEs are safe to apply in this way, since they simply add a conditional
> > check. However, any function change that can not execute safely with
> > the old version of the function can _not_ be safely applied in this
> > version.
>
> [...]
>
> > +/**
> > + * module notifier
> > + */
> > +
> > +static int lp_module_notify(struct notifier_block *nb, unsigned long
> > action,
> > + void *data)
> > +{
> > + struct module *mod = data;
> > + struct lpc_patch *patch;
> > + struct lpc_object *obj;
> > + int ret = 0;
> > +
> > + if (action != MODULE_STATE_COMING)
> > + return 0;
>
> IMHO, we should handle also MODULE_STATE_GOING. We should unregister
> the ftrace handlers and update the state of the affected objects
> (ENABLED -> DISABLED)
The mechanism we use to avoid this right now is taking a reference on
patched module. We only release that reference after the patch is
disabled, which unregisters all the patched functions from ftrace.
However, your comment reminded me of an idea I had to use
MODULE_STATE_GOING and let the lpc_mutex protect against races. I think
it could be cleaner, but I haven't fleshed the idea out fully.
>
> > + down(&lpc_mutex);
> > +
> > + list_for_each_entry(patch, &lpc_patches, list) {
> > + if (patch->state == DISABLED)
> > + continue;
> > + list_for_each_entry(obj, &patch->objs, list) {
> > + if (strcmp(obj->name, mod->name))
> > + continue;
> > + pr_notice("load of module '%s' detected, applying patch
> > '%s'\n",
> > + mod->name, patch->mod->name);
> > + obj->mod = mod;
> > + ret = lpc_enable_object(patch->mod, obj);
> > + if (ret)
> > + goto out;
> > + break;
> > + }
> > + }
> > +
> > + up(&lpc_mutex);
> > + return 0;
> > +out:
>
> I would name this err_our or so to make it clear that it is used when
> something fails.
Just "err" good?
>
> > + up(&lpc_mutex);
> > + WARN("failed to apply patch '%s' to module '%s'\n",
> > + patch->mod->name, mod->name);
> > + return 0;
> > +}
> > +
> > +static struct notifier_block lp_module_nb = {
> > + .notifier_call = lp_module_notify,
> > + .priority = INT_MIN, /* called last */
>
> The handler for MODULE_STATE_COMMING would need have higger priority,
> if we want to cleanly unregister the ftrace handlers.
Yes, we might need two handlers at different priorities if we decide to
go that direction: one for MODULE_STATE_GOING at high/max and one for
MODULE_STATE_COMING at low/min.
Thanks,
Seth
>
> Best Regards,
> Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
more patches for the same func: was Re: [PATCH 2/2] kernel: add support for live patching
On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> This commit introduces code for the live patching core. It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
>
> It represents the greatest common functionality set between kpatch and
> kgraft and can accept patches built using either method.
>
> This first version does not implement any consistency mechanism that
> ensures that old and new code do not run together. In practice, ~90% of
> CVEs are safe to apply in this way, since they simply add a conditional
> check. However, any function change that can not execute safely with
> the old version of the function can _not_ be safely applied in this
> version.
[...]
> +static int lpc_enable_func(struct lpc_func *func)
> +{
> + int ret;
> +
> + BUG_ON(!func->old_addr);
> + BUG_ON(func->state != DISABLED);
> + ret = ftrace_set_filter_ip(&func->fops, func->old_addr, 0, 0);
> + if (ret) {
> + pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> +func->old_name, ret);
> + return ret;
> + }
> + ret = register_ftrace_function(&func->fops);
> + if (ret) {
> + pr_err("failed to register ftrace handler for function '%s'
> (%d)\n",
> +func->old_name, ret);
> + ftrace_set_filter_ip(&func->fops, func->old_addr, 1, 0);
> + } else
> + func->state = ENABLED;
> +
> + return ret;
> +}
> +
[...]
> +/* caller must ensure that obj->mod is set if object is a module */
> +static int lpc_enable_object(struct module *pmod, struct lpc_object *obj)
> +{
> + struct lpc_func *func;
> + int ret;
> +
> + if (obj->mod && !try_module_get(obj->mod))
> + return -ENODEV;
> +
> + if (obj->dynrelas) {
> + ret = lpc_write_object_relocations(pmod, obj);
> + if (ret)
> + goto unregister;
> + }
> + list_for_each_entry(func, &obj->funcs, list) {
> + ret = lpc_find_verify_func_addr(func, obj->name);
> + if (ret)
> + goto unregister;
> +
> + ret = lpc_enable_func(func);
> + if (ret)
> + goto unregister;
> + }
> + obj->state = ENABLED;
> +
> + return 0;
> +unregister:
> + WARN_ON(lpc_unregister_object(obj));
> + return ret;
> +}
> +
> +/**
> + * enable/disable
> + **/
> +
> +/* must be called with lpc_mutex held */
> +static struct lpc_patch *lpc_find_patch(struct lp_patch *userpatch)
> +{
> + struct lpc_patch *patch;
> +
> + list_for_each_entry(patch, &lpc_patches, list)
> + if (patch->userpatch == userpatch)
> + return patch;
> +
> + return NULL;
> +}
[...]
> +
> +/* must be called with lpc_mutex held */
> +static int lpc_enable_patch(struct lpc_patch *patch)
> +{
> + struct lpc_object *obj;
> + int ret;
> +
> + BUG_ON(patch->state != DISABLED);
> +
> + pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> + add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> +
> + pr_notice("enabling patch '%s'\n", patch->mod->name);
> +
> + list_for_each_entry(obj, &patch->objs, list) {
> + if (!is_object_loaded(obj))
> + continue;
> + ret = lpc_enable_object(patch->mod, obj);
> + if (ret)
> + goto unregister;
> + }
> + patch->state = ENABLED;
> + return 0;
> +
> +unregister:
> + WARN_ON(lpc_disable_patch(patch));
> + return ret;
> +}
> +
> +int lp_enable_patch(struct lp_patch *userpatch)
> +{
> + struct lpc_patch *patch;
> + int ret;
> +
> + down(&lpc_mutex);
> + patch = lpc_find_patch(userpatch);
> + if (!patch) {
> + ret = -ENODEV;
> + goto out;
> + }
> + ret = lpc_enable_patch(patch);
> +out:
> + up(&lpc_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(lp_enable_patch);
AFAIK, this does not handle correctly the situation when there
are more patches for the same symbol. IMHO, the first registered
ftrace function wins. It means that later patches are ignored.
In kGraft, we detect this situation and do the following:
add_new_ftrace_function()
/* old one still might be used at this stage */
if (old_function)
remove_old_ftrace_function();
/* the new one is used from now on */
Similar problem is when a patch is disabled. We need to know
if it was actually used. If not, we are done. If it is active,
we need to look if there is an older patch for the the same
symbol and enable the other ftrace function instead.
Best Regards,
Petr
PS: We should probably decide on the used structures before we start
coding fixes for this particular problems. I have similar concern about
the complexity as my colleagues have. But I need to think more about
it. Let'
module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> This commit introduces code for the live patching core. It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
>
> It represents the greatest common functionality set between kpatch and
> kgraft and can accept patches built using either method.
>
> This first version does not implement any consistency mechanism that
> ensures that old and new code do not run together. In practice, ~90% of
> CVEs are safe to apply in this way, since they simply add a conditional
> check. However, any function change that can not execute safely with
> the old version of the function can _not_ be safely applied in this
> version.
[...]
> +/**
> + * module notifier
> + */
> +
> +static int lp_module_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> + struct lpc_patch *patch;
> + struct lpc_object *obj;
> + int ret = 0;
> +
> + if (action != MODULE_STATE_COMING)
> + return 0;
IMHO, we should handle also MODULE_STATE_GOING. We should unregister
the ftrace handlers and update the state of the affected objects
(ENABLED -> DISABLED)
> + down(&lpc_mutex);
> +
> + list_for_each_entry(patch, &lpc_patches, list) {
> + if (patch->state == DISABLED)
> + continue;
> + list_for_each_entry(obj, &patch->objs, list) {
> + if (strcmp(obj->name, mod->name))
> + continue;
> + pr_notice("load of module '%s' detected, applying patch
> '%s'\n",
> + mod->name, patch->mod->name);
> + obj->mod = mod;
> + ret = lpc_enable_object(patch->mod, obj);
> + if (ret)
> + goto out;
> + break;
> + }
> + }
> +
> + up(&lpc_mutex);
> + return 0;
> +out:
I would name this err_our or so to make it clear that it is used when
something fails.
> + up(&lpc_mutex);
> + WARN("failed to apply patch '%s' to module '%s'\n",
> + patch->mod->name, mod->name);
> + return 0;
> +}
> +
> +static struct notifier_block lp_module_nb = {
> + .notifier_call = lp_module_notify,
> + .priority = INT_MIN, /* called last */
The handler for MODULE_STATE_COMMING would need have higger priority,
if we want to cleanly unregister the ftrace handlers.
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 02:13:37PM +0100, Jiri Kosina wrote: > On Fri, 7 Nov 2014, Josh Poimboeuf wrote: > > > > Also, lpc_create_object(), lpc_create_func(), lpc_create_patch(), > > > lpc_create_objects(), lpc_create_funcs(), ... they all are pretty much > > > alike, and are asking for some kind of unification ... perhaps iterator > > > for generic structure initialization? > > > > The allocation and initialization code is very simple and > > straightforward. I really don't see a problem there. > > This really boils down to the question I had in previous mail, whether > three-level hierarchy (patch->object->funcs), which is why there is a lot > of very alike initialization code, is not a bit over-designed. It might right now, but we coded ourselves into a corner a couple of times in kpatch using optimal, but inflexible data structures and sharing those data structures with the API. This structure layout will give us flexibility to make changes without having to gut everything. I see flexibility and modularity being important going forward as we are both looking to extend the abilities. Additionally it allows the sysfs directories to correlate to data structures and we can use the kobject ref count to cleanly do object cleanup (i.e. kobject_put() with release handlers for each ktype). As Josh said, we do have operations that apply to each level. I think your point is that we could do away with the object level, but we have operations that happen on a per-object basis. lpc_enable_object() isn't just a for loop for registering the functions with ftrace. It also does the dynamic relocations. I'm sure we will find other things in the future. It is also nice to have a function that can be called from both lpc_enable_patch() and lp_module_notify() to enable the object in a common way. Thanks, Seth > > > > I am not also really fully convinced that we need the > > > patch->object->funcs abstraction hierarchy (which also contributes to > > > the structure allocation being rather a spaghetti copy/paste code) ... > > > wouldn't patch->funcs be suffcient, with the "object" being made just > > > a property of the function, for example? > > > > > > > Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is > > > > 906+193=1099. I'd say they are about the same size :) > > > > > > Which is still seem to me to be a ratio worth thinking about improving > > > :) > > > > Yes, this code doesn't have a consistency model, but it does have some > > other non-kGraft things like dynamic relocations, > > BTW we need to put those into arch/x86/ as they are unfortunately not > generic. But more on this later independently. > > > deferred module patching, > > FWIW kgraft supports that as well. > > > and a unified API. There's really no point in comparing lines of code. > > Oh, sure, I didn't mean that this is any kind of metrics that should be > taken too seriously at all. I was just expressing my surprise that > unification of the API would bring so much code that it makes the result > comparably sized to "the whole thing" :) > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, Nov 07, 2014 at 02:13:37PM +0100, Jiri Kosina wrote: > On Fri, 7 Nov 2014, Josh Poimboeuf wrote: > > > > Also, lpc_create_object(), lpc_create_func(), lpc_create_patch(), > > > lpc_create_objects(), lpc_create_funcs(), ... they all are pretty much > > > alike, and are asking for some kind of unification ... perhaps iterator > > > for generic structure initialization? > > > > The allocation and initialization code is very simple and > > straightforward. I really don't see a problem there. > > This really boils down to the question I had in previous mail, whether > three-level hierarchy (patch->object->funcs), which is why there is a lot > of very alike initialization code, is not a bit over-designed. Oh sorry, I missed that point :-) See below. > > > > I am not also really fully convinced that we need the > > > patch->object->funcs abstraction hierarchy (which also contributes to > > > the structure allocation being rather a spaghetti copy/paste code) ... > > > wouldn't patch->funcs be suffcient, with the "object" being made just > > > a property of the function, for example? The patched object represents the module being patched (or "vmlinux"). It is much more than a property of the function. Multiple functions can be patched in the same object. There are several things we do on a per-object basis, including try_module_get(), deferred module patching (patching from the module notifier), and dynamic relocations. > > > > > > > Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is > > > > 906+193=1099. I'd say they are about the same size :) > > > > > > Which is still seem to me to be a ratio worth thinking about improving > > > :) > > > > Yes, this code doesn't have a consistency model, but it does have some > > other non-kGraft things like dynamic relocations, > > BTW we need to put those into arch/x86/ as they are unfortunately not > generic. But more on this later independently. > > > deferred module patching, > > FWIW kgraft supports that as well. > > > and a unified API. There's really no point in comparing lines of code. > > Oh, sure, I didn't mean that this is any kind of metrics that should be > taken too seriously at all. I was just expressing my surprise that > unification of the API would bring so much code that it makes the result > comparably sized to "the whole thing" :) > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Fri, 7 Nov 2014, Josh Poimboeuf wrote: > > Also, lpc_create_object(), lpc_create_func(), lpc_create_patch(), > > lpc_create_objects(), lpc_create_funcs(), ... they all are pretty much > > alike, and are asking for some kind of unification ... perhaps iterator > > for generic structure initialization? > > The allocation and initialization code is very simple and > straightforward. I really don't see a problem there. This really boils down to the question I had in previous mail, whether three-level hierarchy (patch->object->funcs), which is why there is a lot of very alike initialization code, is not a bit over-designed. > > I am not also really fully convinced that we need the > > patch->object->funcs abstraction hierarchy (which also contributes to > > the structure allocation being rather a spaghetti copy/paste code) ... > > wouldn't patch->funcs be suffcient, with the "object" being made just > > a property of the function, for example? > > > > > Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is > > > 906+193=1099. I'd say they are about the same size :) > > > > Which is still seem to me to be a ratio worth thinking about improving > > :) > > Yes, this code doesn't have a consistency model, but it does have some > other non-kGraft things like dynamic relocations, BTW we need to put those into arch/x86/ as they are unfortunately not generic. But more on this later independently. > deferred module patching, FWIW kgraft supports that as well. > and a unified API. There's really no point in comparing lines of code. Oh, sure, I didn't mean that this is any kind of metrics that should be taken too seriously at all. I was just expressing my surprise that unification of the API would bring so much code that it makes the result comparably sized to "the whole thing" :) Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 11:20:48PM +0100, Jiri Kosina wrote: > On Thu, 6 Nov 2014, Seth Jennings wrote: > > > > Thanks a lot for having started the work on this! > > > > > > We will be reviewing it carefully in the coming days and will getting > > > back > > > to you (I was surprised to see that that diffstat indicates that it's > > > actually more code than our whole kgraft implementation including the > > > consistency model :) ). > > > > The structure allocation and sysfs stuff is a lot of (mundane) code. > > Lots of boring error path handling too. > > Also, lpc_create_object(), lpc_create_func(), lpc_create_patch(), > lpc_create_objects(), lpc_create_funcs(), ... they all are pretty much > alike, and are asking for some kind of unification ... perhaps iterator > for generic structure initialization? The allocation and initialization code is very simple and straightforward. I really don't see a problem there. Can you give an example of what you mean by "iterator for generic structure initialization"? > I am not also really fully convinced that we need the patch->object->funcs > abstraction hierarchy (which also contributes to the structure allocation > being rather a spaghetti copy/paste code) ... wouldn't patch->funcs be > suffcient, with the "object" being made just a property of the function, > for example? > > > Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is > > 906+193=1099. I'd say they are about the same size :) > > Which is still seem to me to be a ratio worth thinking about improving :) Yes, this code doesn't have a consistency model, but it does have some other non-kGraft things like dynamic relocations, deferred module patching, and a unified API. There's really no point in comparing lines of code. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, 6 Nov 2014, Seth Jennings wrote: > > Thanks a lot for having started the work on this! > > > > We will be reviewing it carefully in the coming days and will getting back > > to you (I was surprised to see that that diffstat indicates that it's > > actually more code than our whole kgraft implementation including the > > consistency model :) ). > > The structure allocation and sysfs stuff is a lot of (mundane) code. > Lots of boring error path handling too. Also, lpc_create_object(), lpc_create_func(), lpc_create_patch(), lpc_create_objects(), lpc_create_funcs(), ... they all are pretty much alike, and are asking for some kind of unification ... perhaps iterator for generic structure initialization? I am not also really fully convinced that we need the patch->object->funcs abstraction hierarchy (which also contributes to the structure allocation being rather a spaghetti copy/paste code) ... wouldn't patch->funcs be suffcient, with the "object" being made just a property of the function, for example? > Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is > 906+193=1099. I'd say they are about the same size :) Which is still seem to me to be a ratio worth thinking about improving :) Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 03:02:04PM -0500, Steven Rostedt wrote: > On Thu, 6 Nov 2014 08:39:08 -0600 > Seth Jennings wrote: > > > --- /dev/null > > +++ b/kernel/livepatch/Kconfig > > @@ -0,0 +1,11 @@ > > +config LIVE_PATCHING > > + tristate "Live Kernel Patching" > > + depends on DYNAMIC_FTRACE_WITH_REGS && MODULES && SYSFS && KALLSYMS_ALL > > + default m > > Nuke this default. This should be default 'n', which is what kconfig > defaults to when none is mentioned. Ok. Thanks, Seth > > -- Steve > > > + help > > + Say Y here if you want to support live kernel patching. > > + This setting has no runtime impact until a live-patch > > + kernel module that uses the live-patch interface provided > > + by this option is loaded, resulting in calls to patched > > + functions being redirected to the new function code contained > > + in the live-patch module. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, 6 Nov 2014 08:39:08 -0600 Seth Jennings wrote: > --- /dev/null > +++ b/kernel/livepatch/Kconfig > @@ -0,0 +1,11 @@ > +config LIVE_PATCHING > + tristate "Live Kernel Patching" > + depends on DYNAMIC_FTRACE_WITH_REGS && MODULES && SYSFS && KALLSYMS_ALL > + default m Nuke this default. This should be default 'n', which is what kconfig defaults to when none is mentioned. -- Steve > + help > + Say Y here if you want to support live kernel patching. > + This setting has no runtime impact until a live-patch > + kernel module that uses the live-patch interface provided > + by this option is loaded, resulting in calls to patched > + functions being redirected to the new function code contained > + in the live-patch module. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 10:20:49AM -0600, Seth Jennings wrote: > Yes, I should explain it. > > This is something that is currently only used in the kpatch approach. > It allows the patching core to do dynamic relocations on the new > function code, similar to what the kernel module linker does, but this > works for non-exported symbols as well. > > This is so the patch module doesn't have to do a kallsyms lookup on > every non-exported symbol that the new functions use. > > The fields of the dynrela structure are those of a normal ELF rela > entry, except for the "external" field, which conveys information about > where the core module should go looking for the symbol referenced in the > dynrela entry. > > Josh was under the impression that Vojtech was ok with putting the > dynrela stuff in the core. Is that not correct (misunderstanding)? Yes, that is correct, as obviously the kpatch way of generating patches by extracting code from a compiled kernel would not be viable without it. For our own kGraft usage we're choosing to compile patches from C source, and there we can simply replace the function calls by calls via pointer looked up through kallsyms. However, kGraft also has tools to create patches in an automated way, where the individual functions are extracted from the compiled patched kernel using a modified objopy and this is hitting exactly the same issue of having to do relocation of unexported symbols if any are referenced. So no objection to the idea. We'll have to look more into the code to comment on the implementation of the dynrela stuff. -- Vojtech Pavlik Director SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 10:57:48AM -0600, Seth Jennings wrote: > On Thu, Nov 06, 2014 at 04:51:02PM +0100, Jiri Slaby wrote: > > On 11/06/2014, 03:39 PM, Seth Jennings wrote: > > > +/* must be called with lpc_mutex held */ > > > +static int lpc_enable_patch(struct lpc_patch *patch) > > > > The question I want to raise here is whether we need two-state > > registration: register+enable. We don't in kGraft. Why do you? > > We actually don't in kpatch either and this was a late change for this > patchset. The thinking was that, while the patch modules would normally > call lpc_register_patch() and lpc_enable_patch() in the same way all the > time, breaking them up created more symmetric code and gives more flexibility > to the API. > > Josh might like to elaborate here. Yes, this was my brilliant idea :-) I like it because it makes the register/unregister interfaces more symmetrical. We already have to separate disable and unregister so that a patch can be disabled from sysfs, so it makes sense IMO to likewise separate register and enable. The downside is an extra function call. The upside is it makes the code cleaner, and the API easier to understand and more flexible. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 04:51:02PM +0100, Jiri Slaby wrote:
> On 11/06/2014, 03:39 PM, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
>
> Hi,
>
> nice! So we have something to start with. Brilliant!
>
> I have some comments below now. Yet, it obviously needs deeper review
> which will take more time.
>
> > --- /dev/null
> > +++ b/include/linux/livepatch.h
> > @@ -0,0 +1,45 @@
> > +#ifndef _LIVEPATCH_H_
> > +#define _LIVEPATCH_H_
>
> This should follow the linux kernel naming: LINUX_LIVEPATCH_H
Didn't realize that was the convention. Just to be sure, you meant
_LINUX_LIVEPATCH_H right (with the leading underscore)?
>
>
> > +#include
> > +
> > +struct lp_func {
>
> I am not much happy with "lp" which effectively means parallel printer
> support. What about lip?
Not sure how much clearer lip is. It isn't for me :-/ I'm not opposed
to changing it. I was just trying to keep the name short since it is
used many times. Reducing the prefix from something like "livepatch_"
to "lp_" seemed to be the shortest and most straightforward way.
>
> > + const char *old_name; /* function to be patched */
> > + void *new_func; /* replacement function in patch module */
> > + /*
> > +* The old_addr field is optional and can be used to resolve
> > +* duplicate symbol names in the vmlinux object. If this
> > +* information is not present, the symbol is located by name
> > +* with kallsyms. If the name is not unique and old_addr is
> > +* not provided, the patch application fails as there is no
> > +* way to resolve the ambiguity.
> > +*/
> > + unsigned long old_addr;
> > +};
> >
> > +struct lp_dynrela {
> > + unsigned long dest;
> > + unsigned long src;
> > + unsigned long type;
> > + const char *name;
> > + int addend;
> > + int external;
> > +};
> > +
> > +struct lp_object {
> > + const char *name; /* "vmlinux" or module name */
> > + struct lp_func *funcs;
> > + struct lp_dynrela *dynrelas;
> > +};
> > +
> > +struct lp_patch {
> > + struct module *mod; /* module containing the patch */
> > + struct lp_object *objs;
> > +};
>
> Please document all the structures and all its members. And use
> kernel-doc format for that. (You can take an inspiration in kgraft.)
Sure.
>
> > +int lp_register_patch(struct lp_patch *);
> > +int lp_unregister_patch(struct lp_patch *);
> > +int lp_enable_patch(struct lp_patch *);
> > +int lp_disable_patch(struct lp_patch *);
> > +
> > +#endif /* _LIVEPATCH_H_ */
>
> ...
>
> > --- /dev/null
> > +++ b/kernel/livepatch/Makefile
> > @@ -0,0 +1,3 @@
> > +obj-$(CONFIG_LIVE_PATCHING) += livepatch.o
> > +
> > +livepatch-objs := core.o
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > new file mode 100644
> > index 000..b32dbb5
> > --- /dev/null
> > +++ b/kernel/livepatch/core.c
> > @@ -0,0 +1,1020 @@
>
> ...
>
> > +/*
> > + * Core structures
> > + /
> > +
> > +/*
> > + * lp_ structs vs lpc_ structs
> > + *
> > + * For each element (patch, object, func) in the live-patching code,
> > + * there are two types with two different prefixes: lp_ and lpc_.
> > + *
> > + * Structures used by the live-patch modules to register with this core
> > module
> > + * are prefixed with lp_ (live patching). These structures are part of the
> > + * registration API and are defined in livepatch.h. The structures used
> > + * internally by this core module are prefixed with lpc_ (live patching
> > core).
> > + */
>
> I am not sure if the separation and the allocations/kobj handling are
> worth it. It makes the code really less understandable. Can we have just
> struct lip_function (don't unnecessarily abbreviate), lip_objectfile
> (object is too generic, like Java object) and lip_patch containing all
> the needed information? It would clean up the code a lot. (Yes, we would
> have profited from c++ here.)
I looked at doing this and this is actually what we did in kpatch. We
made one structure that had "private" members that the user wasn't
suppose to access that were only used in the core. This was messy
though. Every time you wanted to add a "private" field to the struct so
the core could do something new, you were changing the API to the patch
modules as well. While copying the data into an internal structure does
add code and opportunity for errors, that functionality is localized
into functions that are specifically tasked with taking care of that.
So the risk is minimized and we gain flexibility within the core and
more self-documenting API structures.
>
> > +static DEFINE_SEMAPHORE(lpc_mutex);
>
> Ugh, those are deprecated. Use mutex. (Or am I missing the need of
> recursive locking?)
Sure.
>
> > +static LIST_HEAD(lpc_patches);
> > +
> > +enum lpc_state {
> > + D
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 10:20:49AM -0600, Seth Jennings wrote: > On Thu, Nov 06, 2014 at 04:11:37PM +0100, Jiri Kosina wrote: > > On Thu, 6 Nov 2014, Seth Jennings wrote: > > > +/ > > > + * dynamic relocations (load-time linker) > > > + / > > > + > > > +/* > > > + * external symbols are located outside the parent object (where the > > > parent > > > + * object is either vmlinux or the kmod being patched). > > > + */ > > > > I have no ideas what dynrela is, and quickly reading the source doesn't > > really help too much. > > > > Could you please provide some explanation / pointer to some documentation, > > explaining what exactly it is, and why should it be part of the common > > infrastructure? > > Yes, I should explain it. > > This is something that is currently only used in the kpatch approach. > It allows the patching core to do dynamic relocations on the new > function code, similar to what the kernel module linker does, but this > works for non-exported symbols as well. > > This is so the patch module doesn't have to do a kallsyms lookup on > every non-exported symbol that the new functions use. > > The fields of the dynrela structure are those of a normal ELF rela > entry, except for the "external" field, which conveys information about > where the core module should go looking for the symbol referenced in the > dynrela entry. BTW, use of the dynrelas is optional, but highly recommended. The kGraft approach of manually doing a kallsyms lookup for each non-exported symbol is inherently dangerous because of duplicate symbols. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 06, 2014 at 04:11:37PM +0100, Jiri Kosina wrote: > On Thu, 6 Nov 2014, Seth Jennings wrote: > > > This commit introduces code for the live patching core. It implements > > an ftrace-based mechanism and kernel interface for doing live patching > > of kernel and kernel module functions. > > > > It represents the greatest common functionality set between kpatch and > > kgraft and can accept patches built using either method. > > > > This first version does not implement any consistency mechanism that > > ensures that old and new code do not run together. In practice, ~90% of > > CVEs are safe to apply in this way, since they simply add a conditional > > check. However, any function change that can not execute safely with > > the old version of the function can _not_ be safely applied in this > > version. > > Thanks a lot for having started the work on this! > > We will be reviewing it carefully in the coming days and will getting back > to you (I was surprised to see that that diffstat indicates that it's > actually more code than our whole kgraft implementation including the > consistency model :) ). The structure allocation and sysfs stuff is a lot of (mundane) code. Lots of boring error path handling too. Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is 906+193=1099. I'd say they are about the same size :) > > I have one questions right away though. > > > +/ > > + * dynamic relocations (load-time linker) > > + / > > + > > +/* > > + * external symbols are located outside the parent object (where the parent > > + * object is either vmlinux or the kmod being patched). > > + */ > > I have no ideas what dynrela is, and quickly reading the source doesn't > really help too much. > > Could you please provide some explanation / pointer to some documentation, > explaining what exactly it is, and why should it be part of the common > infrastructure? Yes, I should explain it. This is something that is currently only used in the kpatch approach. It allows the patching core to do dynamic relocations on the new function code, similar to what the kernel module linker does, but this works for non-exported symbols as well. This is so the patch module doesn't have to do a kallsyms lookup on every non-exported symbol that the new functions use. The fields of the dynrela structure are those of a normal ELF rela entry, except for the "external" field, which conveys information about where the core module should go looking for the symbol referenced in the dynrela entry. Josh was under the impression that Vojtech was ok with putting the dynrela stuff in the core. Is that not correct (misunderstanding)? Thanks, Seth > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] kernel: add support for live patching
On 11/06/2014, 03:39 PM, Seth Jennings wrote:
> This commit introduces code for the live patching core. It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
Hi,
nice! So we have something to start with. Brilliant!
I have some comments below now. Yet, it obviously needs deeper review
which will take more time.
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,45 @@
> +#ifndef _LIVEPATCH_H_
> +#define _LIVEPATCH_H_
This should follow the linux kernel naming: LINUX_LIVEPATCH_H
> +#include
> +
> +struct lp_func {
I am not much happy with "lp" which effectively means parallel printer
support. What about lip?
> + const char *old_name; /* function to be patched */
> + void *new_func; /* replacement function in patch module */
> + /*
> + * The old_addr field is optional and can be used to resolve
> + * duplicate symbol names in the vmlinux object. If this
> + * information is not present, the symbol is located by name
> + * with kallsyms. If the name is not unique and old_addr is
> + * not provided, the patch application fails as there is no
> + * way to resolve the ambiguity.
> + */
> + unsigned long old_addr;
> +};
>
> +struct lp_dynrela {
> + unsigned long dest;
> + unsigned long src;
> + unsigned long type;
> + const char *name;
> + int addend;
> + int external;
> +};
> +
> +struct lp_object {
> + const char *name; /* "vmlinux" or module name */
> + struct lp_func *funcs;
> + struct lp_dynrela *dynrelas;
> +};
> +
> +struct lp_patch {
> + struct module *mod; /* module containing the patch */
> + struct lp_object *objs;
> +};
Please document all the structures and all its members. And use
kernel-doc format for that. (You can take an inspiration in kgraft.)
> +int lp_register_patch(struct lp_patch *);
> +int lp_unregister_patch(struct lp_patch *);
> +int lp_enable_patch(struct lp_patch *);
> +int lp_disable_patch(struct lp_patch *);
> +
> +#endif /* _LIVEPATCH_H_ */
...
> --- /dev/null
> +++ b/kernel/livepatch/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_LIVE_PATCHING) += livepatch.o
> +
> +livepatch-objs := core.o
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> new file mode 100644
> index 000..b32dbb5
> --- /dev/null
> +++ b/kernel/livepatch/core.c
> @@ -0,0 +1,1020 @@
...
> +/*
> + * Core structures
> + /
> +
> +/*
> + * lp_ structs vs lpc_ structs
> + *
> + * For each element (patch, object, func) in the live-patching code,
> + * there are two types with two different prefixes: lp_ and lpc_.
> + *
> + * Structures used by the live-patch modules to register with this core
> module
> + * are prefixed with lp_ (live patching). These structures are part of the
> + * registration API and are defined in livepatch.h. The structures used
> + * internally by this core module are prefixed with lpc_ (live patching
> core).
> + */
I am not sure if the separation and the allocations/kobj handling are
worth it. It makes the code really less understandable. Can we have just
struct lip_function (don't unnecessarily abbreviate), lip_objectfile
(object is too generic, like Java object) and lip_patch containing all
the needed information? It would clean up the code a lot. (Yes, we would
have profited from c++ here.)
> +static DEFINE_SEMAPHORE(lpc_mutex);
Ugh, those are deprecated. Use mutex. (Or am I missing the need of
recursive locking?)
> +static LIST_HEAD(lpc_patches);
> +
> +enum lpc_state {
> + DISABLED,
> + ENABLED
These are too generic names. This is prone to conflicts in the tree.
> +};
> +
> +struct lpc_func {
> + struct list_head list;
> + struct kobject kobj;
> + struct ftrace_ops fops;
> + enum lpc_state state;
> +
> + const char *old_name;
So you do lpc_func->old_name = lp_func->old_name.
Why? Duplication is always bad and introduces errors. The same for the
other members here and there. Well, lip_function would solve that.
> + unsigned long new_addr;
> + unsigned long old_addr;
> +};
> +
> +struct lpc_object {
> + struct list_head list;
> + struct kobject kobj;
> + struct module *mod; /* module associated with object */
> + enum lpc_state state;
> +
> + const char *name;
> + struct list_head funcs;
> + struct lp_dynrela *dynrelas;
> +};
> +
> +struct lpc_patch {
> + struct list_head list;
> + struct kobject kobj;
> + struct lp_patch *userpatch; /* for correlation during unregister */
> + enum lpc_state state;
> +
> + struct module *mod;
> + struct list_head objs;
> +};
> +
> +/***
> + * Helpers
> + ***/
> +
> +/* sets obj->mod if object is not vmlinux and module was found */
> +static bool is_object_loaded(struct lpc_object *obj)
Always prefix f
Re: [PATCH 2/2] kernel: add support for live patching
On Thu, 6 Nov 2014, Seth Jennings wrote: > This commit introduces code for the live patching core. It implements > an ftrace-based mechanism and kernel interface for doing live patching > of kernel and kernel module functions. > > It represents the greatest common functionality set between kpatch and > kgraft and can accept patches built using either method. > > This first version does not implement any consistency mechanism that > ensures that old and new code do not run together. In practice, ~90% of > CVEs are safe to apply in this way, since they simply add a conditional > check. However, any function change that can not execute safely with > the old version of the function can _not_ be safely applied in this > version. Thanks a lot for having started the work on this! We will be reviewing it carefully in the coming days and will getting back to you (I was surprised to see that that diffstat indicates that it's actually more code than our whole kgraft implementation including the consistency model :) ). I have one questions right away though. > +/ > + * dynamic relocations (load-time linker) > + / > + > +/* > + * external symbols are located outside the parent object (where the parent > + * object is either vmlinux or the kmod being patched). > + */ I have no ideas what dynrela is, and quickly reading the source doesn't really help too much. Could you please provide some explanation / pointer to some documentation, explaining what exactly it is, and why should it be part of the common infrastructure? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] kernel: add support for live patching
This commit introduces code for the live patching core. It implements
an ftrace-based mechanism and kernel interface for doing live patching
of kernel and kernel module functions.
It represents the greatest common functionality set between kpatch and
kgraft and can accept patches built using either method.
This first version does not implement any consistency mechanism that
ensures that old and new code do not run together. In practice, ~90% of
CVEs are safe to apply in this way, since they simply add a conditional
check. However, any function change that can not execute safely with
the old version of the function can _not_ be safely applied in this
version.
Signed-off-by: Seth Jennings
---
MAINTAINERS | 10 +
arch/x86/Kconfig |2 +
include/linux/livepatch.h | 45 ++
kernel/Makefile |1 +
kernel/livepatch/Kconfig | 11 +
kernel/livepatch/Makefile |3 +
kernel/livepatch/core.c | 1020 +
7 files changed, 1092 insertions(+)
create mode 100644 include/linux/livepatch.h
create mode 100644 kernel/livepatch/Kconfig
create mode 100644 kernel/livepatch/Makefile
create mode 100644 kernel/livepatch/core.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f98019e..02d1af7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5671,6 +5671,16 @@ F: Documentation/misc-devices/lis3lv02d
F: drivers/misc/lis3lv02d/
F: drivers/platform/x86/hp_accel.c
+LIVE PATCHING
+M: Josh Poimboeuf
+M: Seth Jennings
+M: Jiri Kosina
+M: Vojtech Pavlik
+S: Maintained
+F: kernel/livepatch/
+F: include/linux/livepatch.h
+L: live-patch...@vger.kernel.org
+
LLC (802.2)
M: Arnaldo Carvalho de Melo
S: Maintained
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9cd2578..fb0bb59 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1982,6 +1982,8 @@ config CMDLINE_OVERRIDE
This is used to work around broken boot loaders. This should
be set to 'N' under normal conditions.
+source "kernel/livepatch/Kconfig"
+
endmenu
config ARCH_ENABLE_MEMORY_HOTPLUG
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
new file mode 100644
index 000..c7a415b
--- /dev/null
+++ b/include/linux/livepatch.h
@@ -0,0 +1,45 @@
+#ifndef _LIVEPATCH_H_
+#define _LIVEPATCH_H_
+
+#include
+
+struct lp_func {
+ const char *old_name; /* function to be patched */
+ void *new_func; /* replacement function in patch module */
+ /*
+* The old_addr field is optional and can be used to resolve
+* duplicate symbol names in the vmlinux object. If this
+* information is not present, the symbol is located by name
+* with kallsyms. If the name is not unique and old_addr is
+* not provided, the patch application fails as there is no
+* way to resolve the ambiguity.
+*/
+ unsigned long old_addr;
+};
+
+struct lp_dynrela {
+ unsigned long dest;
+ unsigned long src;
+ unsigned long type;
+ const char *name;
+ int addend;
+ int external;
+};
+
+struct lp_object {
+ const char *name; /* "vmlinux" or module name */
+ struct lp_func *funcs;
+ struct lp_dynrela *dynrelas;
+};
+
+struct lp_patch {
+ struct module *mod; /* module containing the patch */
+ struct lp_object *objs;
+};
+
+int lp_register_patch(struct lp_patch *);
+int lp_unregister_patch(struct lp_patch *);
+int lp_enable_patch(struct lp_patch *);
+int lp_disable_patch(struct lp_patch *);
+
+#endif /* _LIVEPATCH_H_ */
diff --git a/kernel/Makefile b/kernel/Makefile
index a59481a..616994f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -26,6 +26,7 @@ obj-y += power/
obj-y += printk/
obj-y += irq/
obj-y += rcu/
+obj-y += livepatch/
obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
obj-$(CONFIG_FREEZER) += freezer.o
diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
new file mode 100644
index 000..312ed81
--- /dev/null
+++ b/kernel/livepatch/Kconfig
@@ -0,0 +1,11 @@
+config LIVE_PATCHING
+ tristate "Live Kernel Patching"
+ depends on DYNAMIC_FTRACE_WITH_REGS && MODULES && SYSFS && KALLSYMS_ALL
+ default m
+ help
+ Say Y here if you want to support live kernel patching.
+ This setting has no runtime impact until a live-patch
+ kernel module that uses the live-patch interface provided
+ by this option is loaded, resulting in calls to patched
+ functions being redirected to the new function code contained
+ in the live-patch module.
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
new file mode 100644
index 000..7c1f008
--- /dev/null
+++ b/kernel/livepatch/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_LIVE_PATCHING) += livepatch.o
+
+livepatch-objs := core.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
new file mode 100644
index 000..b32dbb5
--- /dev/null
+
