| Message ID | 1458849640-22588-16-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > Add hook functions which run during patch apply and patch revert. > Hook functions are used by xsplice payloads to manipulate data structures > during patching, etc. Since the added documentation here didn't enlighten me, I've gone back to the design doc, and found a single trivial mentioning of hooks. No example of what they would be useful for, nothing. Unless these can be shown to be needed if any recent XSA fix would be converted to an xSplice patch, I'd recommend dropping this for now. > @@ -851,6 +878,11 @@ static int apply_payload(struct payload *data) > > arch_xsplice_patching_leave(); > > + spin_debug_disable(); > + for ( i = 0; i < data->n_load_funcs; i++ ) > + data->load_funcs[i](); > + spin_debug_enable(); The spin debug disabling needs explanation. And shouldn't this be done before arch_xsplice_patching_leave()? Or wait, documentation above says "before payload is being applied", so it would need to go even further up, and ... > @@ -874,6 +906,11 @@ static int revert_payload(struct payload *data) > > arch_xsplice_patching_leave(); > > + spin_debug_disable(); > + for ( i = 0; i < data->n_unload_funcs; i++ ) > + data->unload_funcs[i](); > + spin_debug_enable(); ... it would be this one which may need to move up by just a few lines. > --- /dev/null > +++ b/xen/include/xen/xsplice_patch.h > @@ -0,0 +1,59 @@ > +/* > + * Copyright (C) 2016 Citrix Systems R&D Ltd. > + */ > + > +#ifndef __XEN_XSPLICE_PATCH_H__ > +#define __XEN_XSPLICE_PATCH_H__ > + > +/* > + * The following definitions are to be used in patches. They are taken > + * from kpatch. > + */ > +typedef void (*xsplice_loadcall_t)(void); > +typedef void (*xsplice_unloadcall_t)(void); Plain function types please. > +/* This definition is taken from Linux. */ > +#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) > +/* > + * XSPLICE_IGNORE_SECTION macro > + * > + * This macro is for ignoring sections that may change as a side effect of > + * another change or might be a non-bundlable section; that is one that does > + * not honor -ffunction-section and create a one-to-one relation from function > + * symbol to section. > + */ > +#define XSPLICE_IGNORE_SECTION(_sec) \ > + char *__UNIQUE_ID(xsplice_ignore_section_) __section(".xsplice.ignore.sections") = _sec; > + > +/* > + * XSPLICE_IGNORE_FUNCTION macro > + * > + * This macro is for ignoring functions that may change as a side effect of a > + * change in another function. > + */ > +#define XSPLICE_IGNORE_FUNCTION(_fn) \ > + void *__xsplice_ignore_func_##_fn __section(".xsplice.ignore.functions") = _fn; Despite mentioned in the commit message, all of the above seems unrelated (and unclear in this context). Even more so that - afaict - they're unusable as we don't seem to have any __PASTE(). Jan
On Fri, Apr 01, 2016 at 09:50:31AM -0600, Jan Beulich wrote: > >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > > > Add hook functions which run during patch apply and patch revert. > > Hook functions are used by xsplice payloads to manipulate data structures > > during patching, etc. > > Since the added documentation here didn't enlighten me, I've gone > back to the design doc, and found a single trivial mentioning of hooks. > No example of what they would be useful for, nothing. Unless these > can be shown to be needed if any recent XSA fix would be converted > to an xSplice patch, I'd recommend dropping this for now. I do like this for the test-case uses (and regression testing). The normal use-case is to modify structures values where we have to be delicate about it and can't just replace the value. As in we may have to recompute the value. > > > @@ -851,6 +878,11 @@ static int apply_payload(struct payload *data) > > > > arch_xsplice_patching_leave(); > > > > + spin_debug_disable(); > > + for ( i = 0; i < data->n_load_funcs; i++ ) > > + data->load_funcs[i](); > > + spin_debug_enable(); > > The spin debug disabling needs explanation. And shouldn't this be > done before arch_xsplice_patching_leave()? Or wait, > documentation above says "before payload is being applied", so it > would need to go even further up, and ... > > > @@ -874,6 +906,11 @@ static int revert_payload(struct payload *data) > > > > arch_xsplice_patching_leave(); > > > > + spin_debug_disable(); > > + for ( i = 0; i < data->n_unload_funcs; i++ ) > > + data->unload_funcs[i](); > > + spin_debug_enable(); > > ... it would be this one which may need to move up by just a few > lines. > > > --- /dev/null > > +++ b/xen/include/xen/xsplice_patch.h > > @@ -0,0 +1,59 @@ > > +/* > > + * Copyright (C) 2016 Citrix Systems R&D Ltd. > > + */ > > + > > +#ifndef __XEN_XSPLICE_PATCH_H__ > > +#define __XEN_XSPLICE_PATCH_H__ > > + > > +/* > > + * The following definitions are to be used in patches. They are taken > > + * from kpatch. > > + */ > > +typedef void (*xsplice_loadcall_t)(void); > > +typedef void (*xsplice_unloadcall_t)(void); > > Plain function types please. Done. > > > +/* This definition is taken from Linux. */ > > +#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) > > +/* > > + * XSPLICE_IGNORE_SECTION macro > > + * > > + * This macro is for ignoring sections that may change as a side effect of > > + * another change or might be a non-bundlable section; that is one that does > > + * not honor -ffunction-section and create a one-to-one relation from function > > + * symbol to section. > > + */ > > +#define XSPLICE_IGNORE_SECTION(_sec) \ > > + char *__UNIQUE_ID(xsplice_ignore_section_) __section(".xsplice.ignore.sections") = _sec; > > + > > +/* > > + * XSPLICE_IGNORE_FUNCTION macro > > + * > > + * This macro is for ignoring functions that may change as a side effect of a > > + * change in another function. > > + */ > > +#define XSPLICE_IGNORE_FUNCTION(_fn) \ > > + void *__xsplice_ignore_func_##_fn __section(".xsplice.ignore.functions") = _fn; > > Despite mentioned in the commit message, all of the above seems > unrelated (and unclear in this context). Even more so that - afaict - > they're unusable as we don't seem to have any __PASTE(). /me nods. I will remove them. > > Jan >
On 06.04.2016 04:42, Konrad Rzeszutek Wilk wrote: > On Fri, Apr 01, 2016 at 09:50:31AM -0600, Jan Beulich wrote: >>>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >>> From: Ross Lagerwall <ross.lagerwall@citrix.com> >>> >>> Add hook functions which run during patch apply and patch revert. >>> Hook functions are used by xsplice payloads to manipulate data structures >>> during patching, etc. >> >> Since the added documentation here didn't enlighten me, I've gone >> back to the design doc, and found a single trivial mentioning of hooks. >> No example of what they would be useful for, nothing. Unless these >> can be shown to be needed if any recent XSA fix would be converted >> to an xSplice patch, I'd recommend dropping this for now. > > I do like this for the test-case uses (and regression testing). > > The normal use-case is to modify structures values where we have to > be delicate about it and can't just replace the value. As in we > may have to recompute the value. Agree on the default use (e.g., for XSA 91). If we have shadow variables, we also need an unload hook to garbage collect all the variables introduced by a hotpatch to prevent memory leaks. Potentially, we also want to pre-reserve memory for static or existing dynamic objects in the load-hook instead of on the fly. For testing and debugging, various applications are possible. In general, the hooks provide flexibility when having to deal with unforeseen cases, but their application should be rarely required (< 10%). I would argue that we can delay them until v2, together with the shadow variables Martin Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 04/06/16 4:44 AM >>> >On Fri, Apr 01, 2016 at 09:50:31AM -0600, Jan Beulich wrote: >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >> > From: Ross Lagerwall <ross.lagerwall@citrix.com> >> > >> > Add hook functions which run during patch apply and patch revert. >> > Hook functions are used by xsplice payloads to manipulate data structures >> > during patching, etc. >> >> Since the added documentation here didn't enlighten me, I've gone >> back to the design doc, and found a single trivial mentioning of hooks. >> No example of what they would be useful for, nothing. Unless these >> can be shown to be needed if any recent XSA fix would be converted >> to an xSplice patch, I'd recommend dropping this for now. > >I do like this for the test-case uses (and regression testing). I don't think having infrastructure just for test cases' sake is a good idea. >The normal use-case is to modify structures values where we have to >be delicate about it and can't just replace the value. As in we >may have to recompute the value. That's the normal _theoretical_ use case afaict. Or else you would have given an example as asked for above. Jan
>>> Martin Pohlack <mpohlack@amazon.com> 04/06/16 8:42 AM >>> >On 06.04.2016 04:42, Konrad Rzeszutek Wilk wrote: >> The normal use-case is to modify structures values where we have to >> be delicate about it and can't just replace the value. As in we >> may have to recompute the value. > >Agree on the default use (e.g., for XSA 91). XSA-91 was an ARM one, i.e. would still only be a theoretical example for the purpose of the discussion here. Jan
On 04/07/2016 02:15 AM, Jan Beulich wrote: >>>> Martin Pohlack <mpohlack@amazon.com> 04/06/16 8:42 AM >>> >> On 06.04.2016 04:42, Konrad Rzeszutek Wilk wrote: >>> The normal use-case is to modify structures values where we have to >>> be delicate about it and can't just replace the value. As in we >>> may have to recompute the value. >> >> Agree on the default use (e.g., for XSA 91). > > XSA-91 was an ARM one, i.e. would still only be a theoretical example for > the purpose of the discussion here. > > Jan > I've marked the following XSAs as potentially requiring hook functions or shadow variables: XSA-36 XSA-45 XSA-60 XSA-64 XSA-80 XSA-82 XSA-97 XSA-107 XSA-114 XSA-150
>>> On 08.04.16 at 17:57, <ross.lagerwall@citrix.com> wrote: > On 04/07/2016 02:15 AM, Jan Beulich wrote: >>>>> Martin Pohlack <mpohlack@amazon.com> 04/06/16 8:42 AM >>> >>> On 06.04.2016 04:42, Konrad Rzeszutek Wilk wrote: >>>> The normal use-case is to modify structures values where we have to >>>> be delicate about it and can't just replace the value. As in we >>>> may have to recompute the value. >>> >>> Agree on the default use (e.g., for XSA 91). >> >> XSA-91 was an ARM one, i.e. would still only be a theoretical example for >> the purpose of the discussion here. > > I've marked the following XSAs as potentially requiring hook functions > or shadow variables: > > XSA-36 > XSA-45 > XSA-60 > XSA-64 > XSA-80 > XSA-82 > XSA-97 > XSA-107 > XSA-114 > XSA-150 Such a raw list doesn't tell me anything, I'm afraid. Can you please explain for at least one of them why they do need hooks (and not just potentially)? Jan
On 04/08/2016 06:39 PM, Jan Beulich wrote: >>>> On 08.04.16 at 17:57, <ross.lagerwall@citrix.com> wrote: >> On 04/07/2016 02:15 AM, Jan Beulich wrote: >>>>>> Martin Pohlack <mpohlack@amazon.com> 04/06/16 8:42 AM >>> >>>> On 06.04.2016 04:42, Konrad Rzeszutek Wilk wrote: >>>>> The normal use-case is to modify structures values where we have to >>>>> be delicate about it and can't just replace the value. As in we >>>>> may have to recompute the value. >>>> >>>> Agree on the default use (e.g., for XSA 91). >>> >>> XSA-91 was an ARM one, i.e. would still only be a theoretical example for >>> the purpose of the discussion here. >> >> I've marked the following XSAs as potentially requiring hook functions >> or shadow variables: >> >> XSA-36 >> XSA-45 >> XSA-60 >> XSA-64 >> XSA-80 >> XSA-82 >> XSA-97 >> XSA-107 >> XSA-114 >> XSA-150 > > Such a raw list doesn't tell me anything, I'm afraid. Can you please > explain for at least one of them why they do need hooks (and not > just potentially)? > Some examples: XSA-80: In addition to patching the code, a hook function is needed to set iommu_dont_flush_iotlb back to 0. XSA-82: The original patch applies to an __init function. Instead, use a hook function to update the MSR. Of course, it requires someone with decent knowledge of the code area & source patch to determine exactly what needs to be done. But any patch which touches initialization code (__init or otherwise) or changes the layout or content of data (structs or static/global data) will either need the source patch to be rewritten and/or the use of hook functions/shadow variables. The XSAs I've listed above fall into this category.
>>> On 11.04.16 at 10:23, <ross.lagerwall@citrix.com> wrote: > Some examples: > XSA-80: In addition to patching the code, a hook function is needed to > set iommu_dont_flush_iotlb back to 0. I don't think this is an issue that can be reasonably life patched: Doing so would likely lead to the false impression that everything is fine after the patching, which it isn't as you don't know what other flushing didn't happen as needed, and hence what else is in a broken state at the point of patching. > XSA-82: The original patch applies to an __init function. Instead, use a > hook function to update the MSR. __devinit == <nothing> (i.e. != __init) The right way to deal with MSR value changes post boot imo is to run a utility from Dom0 user mode which fiddles with the MSR on all CPUs. No need for a patch to also deal with machine state. Jan
>>> On 11.04.16 at 10:23, <ross.lagerwall@citrix.com> wrote: > On 04/08/2016 06:39 PM, Jan Beulich wrote: >>>>> On 08.04.16 at 17:57, <ross.lagerwall@citrix.com> wrote: >>> I've marked the following XSAs as potentially requiring hook functions >>> or shadow variables: >>> >>> XSA-36 Again an example that I don't think can be live patched: The ACPI tables the parsing of which gets adjusted may not be available anymore at the time of patching. >>> XSA-45 Hmm, this one is adding a field to struct vcpu, which by itself already makes it very difficult to deal with this correctly in a patch. But yes, to construct such a beast (if that's possible at all), I can see how either or both of the above could be useful here. >>> XSA-60 While the field additions could be dealt with here, this again is so complex a change that I personally wouldn't dare to recommend using a live patch for this, even if someone managed to create one. >>> XSA-64 I don't think this can be fixed for any guests already started, as it doesn't look like simply going and zapping the mis-initialized L4 entries would actually be correct in all cases. >>> XSA-97 A field addition to an existing structure again - see above. >>> XSA-107 Yes, this indeed could be dealt with in a hook (but I can also see ways to deal with this without). >>> XSA-114 Hmm, yes, if you really manage to enumerate all r/w locks in the system, this could be dealt with in a hook too. >>> XSA-150 Growing a structure again, so see above. Overall I think that all of the cited examples are such which already don't really lend themselves to live patching. Hence I think we're going to be fine without these extra two pieces for the initial round, taking into consideration just those cases where live patching is reasonable to do. Jan
> Overall I think that all of the cited examples are such which already > don't really lend themselves to live patching. Hence I think we're > going to be fine without these extra two pieces for the initial round, /me nods. > taking into consideration just those cases where live patching is > reasonable to do. I've dropped them from my v1 patchset. > > Jan >
diff --git a/docs/misc/xsplice.markdown b/docs/misc/xsplice.markdown index a498a61..be9cd71 100644 --- a/docs/misc/xsplice.markdown +++ b/docs/misc/xsplice.markdown @@ -285,6 +285,12 @@ like what the Linux kernel module loader does. The payload contains a section (xsplice_patch_func) with an array of structures describing the functions to be patched: +It optionally may contain the address of functions to be called right before +being applied and after being reverted: + + * `.xsplice.hooks.load` - an array of function pointers. + * `.xsplice.hooks.unload` - an array of function pointers. + <pre> struct xsplice_patch_func { @@ -362,6 +368,21 @@ struct xsplice_patch_func xsplice_hello_world = { Code must be compiled with -fPIC. + +### .xsplice.hooks.load and .xsplice.hooks.unload + +This section contains an array of function pointers to be executed +before payload is being applied (.xsplice.funcs) or after reverting +the payload. + +Each entry in this array is eight bytes. + +The type definition of the function are as follow: + +<pre> +typedef void (*xsplice_loadcall_t)(void); +typedef void (*xsplice_unloadcall_t)(void); +</pre> ## Hypercalls We will employ the sub operations of the system management hypercall (sysctl). diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c index 243eb3f..d2b3cc2 100644 --- a/xen/arch/x86/test/xen_hello_world.c +++ b/xen/arch/x86/test/xen_hello_world.c @@ -5,8 +5,10 @@ #include <xen/config.h> #include <xen/types.h> +#include <xen/xsplice_patch.h> #include <xen/xsplice.h> #include "config.h" +#include <xen/lib.h> static char xen_hello_world_name[] = "xen_hello_world"; extern const char *xen_hello_world(void); @@ -14,6 +16,19 @@ extern const char *xen_hello_world(void); /* External symbol. */ extern const char *xen_extra_version(void); +void apply_hook(void) +{ + printk(KERN_DEBUG "Hook executing.\n"); +} + +void revert_hook(void) +{ + printk(KERN_DEBUG "Hook unloaded.\n"); +} + +XSPLICE_LOAD_HOOK(apply_hook); +XSPLICE_UNLOAD_HOOK(revert_hook); + struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world = { .name = xen_hello_world_name, .new_addr = (unsigned long)(xen_hello_world), diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c index 5e77360..03b32e4 100644 --- a/xen/common/xsplice.c +++ b/xen/common/xsplice.c @@ -21,6 +21,7 @@ #include <xen/wait.h> #include <xen/xsplice_elf.h> #include <xen/xsplice.h> +#include <xen/xsplice_patch.h> #include <asm/event.h> #include <asm/nmi.h> @@ -62,6 +63,10 @@ struct payload { struct xsplice_symbol *symtab; /* All symbols. */ char *strtab; /* Pointer to .strtab. */ unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ + xsplice_loadcall_t *load_funcs; /* The array of funcs to call after */ + xsplice_unloadcall_t *unload_funcs; /* load and unload of the payload. */ + unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */ + unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */ char name[XEN_XSPLICE_NAME_SIZE]; /* Name of it. */ }; @@ -499,6 +504,28 @@ static int prepare_payload(struct payload *payload, } } + sec = xsplice_elf_sec_by_name(elf, ".xsplice.hooks.load"); + if ( sec ) + { + if ( !sec->sec->sh_size || + (sec->sec->sh_size % sizeof (*payload->load_funcs)) ) + return -EINVAL; + + payload->load_funcs = (xsplice_loadcall_t *)sec->load_addr; + payload->n_load_funcs = sec->sec->sh_size / (sizeof *payload->load_funcs); + } + + sec = xsplice_elf_sec_by_name(elf, ".xsplice.hooks.unload"); + if ( sec ) + { + if ( !sec->sec->sh_size || + (sec->sec->sh_size % sizeof (*payload->unload_funcs)) ) + return -EINVAL; + + payload->unload_funcs = (xsplice_unloadcall_t *)sec->load_addr; + payload->n_unload_funcs = sec->sec->sh_size / (sizeof *payload->unload_funcs); + } + /* Setup the virtual region with proper data. */ region = &payload->region; @@ -851,6 +878,11 @@ static int apply_payload(struct payload *data) arch_xsplice_patching_leave(); + spin_debug_disable(); + for ( i = 0; i < data->n_load_funcs; i++ ) + data->load_funcs[i](); + spin_debug_enable(); + list_add_tail(&data->applied_list, &applied_list); register_virtual_region(&data->region); @@ -874,6 +906,11 @@ static int revert_payload(struct payload *data) arch_xsplice_patching_leave(); + spin_debug_disable(); + for ( i = 0; i < data->n_unload_funcs; i++ ) + data->unload_funcs[i](); + spin_debug_enable(); + list_del_init(&data->applied_list); unregister_virtual_region(&data->region); diff --git a/xen/include/xen/xsplice_patch.h b/xen/include/xen/xsplice_patch.h new file mode 100644 index 0000000..19d3f76 --- /dev/null +++ b/xen/include/xen/xsplice_patch.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2016 Citrix Systems R&D Ltd. + */ + +#ifndef __XEN_XSPLICE_PATCH_H__ +#define __XEN_XSPLICE_PATCH_H__ + +/* + * The following definitions are to be used in patches. They are taken + * from kpatch. + */ +typedef void (*xsplice_loadcall_t)(void); +typedef void (*xsplice_unloadcall_t)(void); + +/* This definition is taken from Linux. */ +#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) +/* + * XSPLICE_IGNORE_SECTION macro + * + * This macro is for ignoring sections that may change as a side effect of + * another change or might be a non-bundlable section; that is one that does + * not honor -ffunction-section and create a one-to-one relation from function + * symbol to section. + */ +#define XSPLICE_IGNORE_SECTION(_sec) \ + char *__UNIQUE_ID(xsplice_ignore_section_) __section(".xsplice.ignore.sections") = _sec; + +/* + * XSPLICE_IGNORE_FUNCTION macro + * + * This macro is for ignoring functions that may change as a side effect of a + * change in another function. + */ +#define XSPLICE_IGNORE_FUNCTION(_fn) \ + void *__xsplice_ignore_func_##_fn __section(".xsplice.ignore.functions") = _fn; + +/* + * XSPLICE_LOAD_HOOK macro + * + * Declares a function pointer to be allocated in a new + * .xsplice.hook.load section. This xsplice_load_data symbol is later + * stripped by create-diff-object so that it can be declared in multiple + * objects that are later linked together, avoiding global symbol + * collision. Since multiple hooks can be registered, the + * .xsplice.hook.load section is a table of functions that will be + * executed in series by the xsplice infrastructure at patch load time. + */ +#define XSPLICE_LOAD_HOOK(_fn) \ + xsplice_loadcall_t __attribute__((weak)) xsplice_load_data __section(".xsplice.hooks.load") = _fn; + +/* + * XSPLICE_UNLOAD_HOOK macro + * + * Same as LOAD hook with s/load/unload/ + */ +#define XSPLICE_UNLOAD_HOOK(_fn) \ + xsplice_unloadcall_t __attribute__((weak)) xsplice_unload_data __section(".xsplice.hooks.unload") = _fn; + +#endif /* __XEN_XSPLICE_PATCH_H__ */