diff mbox

[v3,9/9] livepach: Add .livepatch.hooks functions and test-case

Message ID 1471211546-2235-10-git-send-email-konrad.wilk@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Aug. 14, 2016, 9:52 p.m. UTC
From: Ross Lagerwall <ross.lagerwall@citrix.com>

Add hook functions which run during patch apply and patch revert.
Hook functions are used by livepatch payloads to manipulate data
structures during patching, etc.

One use case is the XSA91. As Martin mentions it:
"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%)."

Furthermore include a test-case for it.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

v4..v11: Defered for v4.8
v12: s/xsplice/livepatch/
v13: Clarify the comments about spin_debug_enable
     Rename one of the hooks to lower-case (Z->z) to guarantee it being
     called last.
     Learned a lot of about 'const'
---
 docs/misc/livepatch.markdown        | 23 +++++++++++++++++
 xen/arch/x86/test/xen_hello_world.c | 35 ++++++++++++++++++++++++++
 xen/common/livepatch.c              | 50 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/xen/livepatch_payload.h

Comments

Jan Beulich Aug. 15, 2016, 11:15 a.m. UTC | #1
>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> v4..v11: Defered for v4.8
> v12: s/xsplice/livepatch/
> v13: Clarify the comments about spin_debug_enable

(Side note: v13 here vs v3 in the subject.)

>      Rename one of the hooks to lower-case (Z->z) to guarantee it being
>      called last.

Does lower case z really guarantee that? Wouldn't it be better to
use a sort order independent mechanism, like using another object
file (iirc object file order defines placement-within-sections unless
options get handed to the linker which specifically allow it to
shuffle things around)?

> @@ -72,7 +73,11 @@ struct payload {
>      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
>      void *bss;                           /* .bss of the payload. */
>      size_t bss_size;                     /* and its size. */
> -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
> +    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call after */
> +    livepatch_unloadcall_t **unload_funcs;/* load and unload of the payload. */

Considering above you said "Learned a lot of about 'const'", where
are they? (Interestingly, LIVEPATCH_{,UN}LOAD_HOOK() below look
correct now, so effectively you lose constness here.)

> @@ -1065,6 +1089,18 @@ static int apply_payload(struct payload *data)
>  
>      arch_livepatch_quiesce();
>  
> +    /*
> +     * Since we are running with IRQs disabled and the hooks may call common
> +     * code - which expects the spinlocks to run with IRQs enabled - we temporarly
> +     * disable the spin locks IRQ state checks.
> +     */

Much better, but a little further to go: I'd suggest
s/the spinlocks/certain spinlocks/ or some such. Also - "temporarily".

Jan
Konrad Rzeszutek Wilk Aug. 15, 2016, 2:46 p.m. UTC | #2
On Mon, Aug 15, 2016 at 05:15:28AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> > v4..v11: Defered for v4.8
> > v12: s/xsplice/livepatch/
> > v13: Clarify the comments about spin_debug_enable
> 
> (Side note: v13 here vs v3 in the subject.)
> 
> >      Rename one of the hooks to lower-case (Z->z) to guarantee it being
> >      called last.
> 
> Does lower case z really guarantee that? Wouldn't it be better to
> use a sort order independent mechanism, like using another object
> file (iirc object file order defines placement-within-sections unless
> options get handed to the linker which specifically allow it to
> shuffle things around)?
> 
> > @@ -72,7 +73,11 @@ struct payload {
> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> >      void *bss;                           /* .bss of the payload. */
> >      size_t bss_size;                     /* and its size. */
> > -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
> > +    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call after */
> > +    livepatch_unloadcall_t **unload_funcs;/* load and unload of the payload. */
> 
> Considering above you said "Learned a lot of about 'const'", where
> are they? (Interestingly, LIVEPATCH_{,UN}LOAD_HOOK() below look
> correct now, so effectively you lose constness here.)

Can't do const here at all. Any placement of them will make the compile
omit the call to them.

That is either one of:

 const livepatch_loadcall_t **load_funcs;
 livepatch_loadcall_t const **load_funcs;

results in (on gcc-5.2.1):
       .loc 1 1152 0
        call    spin_debug_disable
.LVL69: 
        .loc 1 1153 0
        movl    324(%r12), %edx
        testl   %edx, %edx
        je      .L32
        movl    $0, %eax
.LVL70:
.L33:   
        .loc 1 1153 0 is_stmt 0 discriminator 3
        addl    $1, %eax
.LVL71: 
        cmpl    %edx, %eax
        jne     .L33
.LVL72:
.L32:   
        .loc 1 1155 0 is_stmt 1
        call    spin_debug_enable

(on older compiler 4.4.4) I get:
.L122:
    .loc 1 1108 0
    call    spin_debug_disable
.LVL175:
    .loc 1 1111 0
    .p2align 4,,8
    call    spin_debug_enable


> 
> > @@ -1065,6 +1089,18 @@ static int apply_payload(struct payload *data)
> >  
> >      arch_livepatch_quiesce();
> >  
> > +    /*
> > +     * Since we are running with IRQs disabled and the hooks may call common
> > +     * code - which expects the spinlocks to run with IRQs enabled - we temporarly
> > +     * disable the spin locks IRQ state checks.
> > +     */
> 
> Much better, but a little further to go: I'd suggest
> s/the spinlocks/certain spinlocks/ or some such. Also - "temporarily".
> 
> Jan
>
Jan Beulich Aug. 15, 2016, 3:17 p.m. UTC | #3
>>> On 15.08.16 at 16:46, <konrad.wilk@oracle.com> wrote:
> On Mon, Aug 15, 2016 at 05:15:28AM -0600, Jan Beulich wrote:
>> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>> > @@ -72,7 +73,11 @@ struct payload {
>> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
>> >      void *bss;                           /* .bss of the payload. */
>> >      size_t bss_size;                     /* and its size. */
>> > -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
>> > +    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call after */
>> > +    livepatch_unloadcall_t **unload_funcs;/* load and unload of the payload. */
>> 
>> Considering above you said "Learned a lot of about 'const'", where
>> are they? (Interestingly, LIVEPATCH_{,UN}LOAD_HOOK() below look
>> correct now, so effectively you lose constness here.)
> 
> Can't do const here at all. Any placement of them will make the compile
> omit the call to them.
> 
> That is either one of:
> 
>  const livepatch_loadcall_t **load_funcs;
>  livepatch_loadcall_t const **load_funcs;

These two (functionally identical) variants aren't exhaustive for
"any placement"; the right one ought to be

 livepatch_loadcall_t *const *load_funcs;

(which is what I tried to clarify by saying "in the middle" in the original
reply).

Jan
diff mbox

Patch

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 590757d..0d3da21 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -342,6 +342,13 @@  When reverting a patch, the hypervisor iterates over each `livepatch_func`
 and the core code copies the data from the undo buffer (private internal copy)
 to `old_addr`.
 
+It optionally may contain the address of functions to be called right before
+being applied and after being reverted:
+
+ * `.livepatch.hooks.load` - an array of function pointers.
+ * `.livepatch.hooks.unload` - an array of function pointers.
+
+
 ### Example of .livepatch.funcs
 
 A simple example of what a payload file can be:
@@ -379,6 +386,22 @@  struct livepatch_func livepatch_hello_world = {
 
 Code must be compiled with -fPIC.
 
+### .livepatch.hooks.load and .livepatch.hooks.unload
+
+This section contains an array of function pointers to be executed
+before payload is being applied (.livepatch.funcs) or after reverting
+the payload. This is useful to prepare data structures that need to
+be modified patching.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+<pre>
+typedef void (*livepatch_loadcall_t)(void);  
+typedef void (*livepatch_unloadcall_t)(void);   
+</pre>
+
 ### .livepatch.depends and .note.gnu.build-id
 
 To support dependencies checking and safe loading (to load the
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index 422bdf1..66c5b8e 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -4,14 +4,49 @@ 
  */
 
 #include "config.h"
+#include <xen/lib.h>
 #include <xen/types.h>
 #include <xen/version.h>
 #include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
 
 #include <public/sysctl.h>
 
 static char hello_world_patch_this_fnc[] = "xen_extra_version";
 extern const char *xen_hello_world(void);
+static unsigned int cnt;
+
+static void apply_hook(void)
+{
+    printk(KERN_DEBUG "Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+    printk(KERN_DEBUG "Hook unloaded.\n");
+}
+
+static void  hi_func(void)
+{
+    printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
+};
+
+/* If we are sorted we _MUST_ be the last .livepatch.hook section. */
+static void z_check_fnc(void)
+{
+    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
+    BUG_ON(cnt == 0 || cnt > 2);
+}
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+/* Imbalance here. Two load and three unload. */
+
+LIVEPATCH_LOAD_HOOK(hi_func);
+LIVEPATCH_UNLOAD_HOOK(hi_func);
+
+LIVEPATCH_UNLOAD_HOOK(z_check_fnc);
 
 struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
     .version = LIVEPATCH_PAYLOAD_VERSION,
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 6f82a9e..3a22de2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -23,6 +23,7 @@ 
 #include <xen/wait.h>
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
 
 #include <asm/event.h>
 
@@ -72,7 +73,11 @@  struct payload {
     struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
     void *bss;                           /* .bss of the payload. */
     size_t bss_size;                     /* and its size. */
-    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
+    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call after */
+    livepatch_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_LIVEPATCH_NAME_SIZE];  /* Name of it. */
 };
 
 /* Defines an outstanding patching action. */
@@ -581,6 +586,25 @@  static int prepare_payload(struct payload *payload,
             return rc;
     }
 
+    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
+    if ( sec )
+    {
+        if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
+            return -EINVAL;
+
+        payload->load_funcs = sec->load_addr;
+        payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
+    }
+
+    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
+    if ( sec )
+    {
+        if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
+            return -EINVAL;
+
+        payload->unload_funcs = sec->load_addr;
+        payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
+    }
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
     if ( sec )
     {
@@ -1065,6 +1089,18 @@  static int apply_payload(struct payload *data)
 
     arch_livepatch_quiesce();
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects the spinlocks to run with IRQs enabled - we temporarly
+     * disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
+    for ( i = 0; i < data->n_load_funcs; i++ )
+        data->load_funcs[i]();
+    spin_debug_enable();
+
+    ASSERT(!local_irq_is_enabled());
+
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_apply_jmp(&data->funcs[i]);
 
@@ -1091,6 +1127,18 @@  static int revert_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_revert_jmp(&data->funcs[i]);
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects the spinlocks to run with IRQs enabled - we temporarly
+     * disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
+    for ( i = 0; i < data->n_unload_funcs; i++ )
+        data->unload_funcs[i]();
+    spin_debug_enable();
+
+    ASSERT(!local_irq_is_enabled());
+
     arch_livepatch_revive();
 
     /*
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
new file mode 100644
index 0000000..8f38cc2
--- /dev/null
+++ b/xen/include/xen/livepatch_payload.h
@@ -0,0 +1,49 @@ 
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#ifndef __XEN_LIVEPATCH_PAYLOAD_H__
+#define __XEN_LIVEPATCH_PAYLOAD_H__
+
+/*
+ * The following definitions are to be used in patches. They are taken
+ * from kpatch.
+ */
+typedef void livepatch_loadcall_t(void);
+typedef void livepatch_unloadcall_t(void);
+
+/*
+ * LIVEPATCH_LOAD_HOOK macro
+ *
+ * Declares a function pointer to be allocated in a new
+ * .livepatch.hook.load section.  This livepatch_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
+ * .livepatch.hook.load section is a table of functions that will be
+ * executed in series by the livepatch infrastructure at patch load time.
+ */
+#define LIVEPATCH_LOAD_HOOK(_fn) \
+    livepatch_loadcall_t *__attribute__((weak)) \
+        const livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
+
+/*
+ * LIVEPATCH_UNLOAD_HOOK macro
+ *
+ * Same as LOAD hook with s/load/unload/
+ */
+#define LIVEPATCH_UNLOAD_HOOK(_fn) \
+     livepatch_unloadcall_t *__attribute__((weak)) \
+        const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
+
+#endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */