[PATCH v1] Livepatch patches for v4.8 (v1)

[PATCH v1] Livepatch patches for v4.8 (v1)

[Konrad Rzeszutek Wilk]

Attached are thee patches for v4.8. The first one had been posted
during v4.7 but we decided to punt to v4.8:
 [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

The other one is to make it easier to make sure that the livepatch
symbols match with the hypervisor one and not have to figure this
out when loading the payloads:
 [PATCH v2 2/3] symbols: Generate an xen-sym.map

And the last one is a bug-fix:
 [PATCH v2 3/3] livepatch: Sync cache of build-id before using it


 .gitignore                          |  1 +
 docs/misc/livepatch.markdown        | 23 +++++++++++++++++
 xen/Makefile                        |  4 ++-
 xen/arch/arm/Makefile               |  3 +++
 xen/arch/x86/Makefile               |  4 +++
 xen/arch/x86/test/xen_hello_world.c | 36 ++++++++++++++++++++++++++
 xen/common/livepatch.c              | 51 ++++++++++++++++++++++++++++++++++++-
 xen/common/version.c                |  6 ++++-
 xen/include/xen/livepatch.h         |  2 +-
 xen/include/xen/livepatch_payload.h | 49 +++++++++++++++++++++++++++++++++++
 xen/tools/symbols.c                 | 12 ++++++++-
 11 files changed, 186 insertions(+), 5 deletions(-)


Konrad Rzeszutek Wilk (2):
      symbols: Generate an xen-sym.map
      livepatch: Sync cache of build-id before using it first time.

Ross Lagerwall (1):
      livepach: Add .livepatch.hooks functions and test-case


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

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>
---
v12: s/xsplice/livepatch/

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>
---
 docs/misc/livepatch.markdown        | 23 +++++++++++++++++
 xen/arch/x86/test/xen_hello_world.c | 36 ++++++++++++++++++++++++++
 xen/common/livepatch.c              | 50 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/xen/livepatch_payload.h

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..e0fc5e4 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -340,6 +340,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:
@@ -377,6 +384,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..e6a095b 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -4,14 +4,50 @@
  */
 
 #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);
+    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 4! */
+}
+
+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 5da28a3..f6dbd51 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>
 
@@ -70,7 +71,11 @@ struct payload {
     unsigned int nsyms;                  /* Nr of entries in .strtab and symbols. */
     struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
     struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
-    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. */
@@ -515,6 +520,27 @@ static int prepare_payload(struct payload *payload,
         }
     }
 
+    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
+    if ( sec )
+    {
+        if ( !sec->sec->sh_size ||
+             (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 ||
+             (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 )
     {
@@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
 
     arch_livepatch_quiesce();
 
+    /*
+     * The hooks may call common code which expects spinlocks to be certain
+     * type, as such disable this temporarily.
+     */
+    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]);
 
@@ -1025,6 +1062,17 @@ static int revert_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_revert_jmp(&data->funcs[i]);
 
+    /*
+     * The hooks may call common code which expects spinlocks to be certain
+     * type, as such disable this temporarily.
+     */
+    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..cebf568
--- /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)) \
+        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)) \
+        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:
+ */
-- 
2.5.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
> In general, the hooks provide flexibility when having to deal with
> unforeseen cases, but their application should be rarely required (<
> 10%)."

But the greater flexibility of course comes with increased chances
of screwing things up. I'm therefore still not entirely convinced that
such XSAs wouldn't then better not be live patched.

> --- a/xen/arch/x86/test/xen_hello_world.c
> +++ b/xen/arch/x86/test/xen_hello_world.c
> @@ -4,14 +4,50 @@
>   */
>  
>  #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)

And that Z_ prefix is supposed to guarantee that being last? Isn't
it that upper case letters sort before lower case ones?

> +{
> +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
> +    BUG_ON(cnt == 0 || cnt > 2);
> +    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 4! */

Isn't this an indication of a general weakness: Shouldn't a patch
module be allowed to assume it's being run in a clean state, i.e.
without any of its static data altered from their load time values?

> @@ -70,7 +71,11 @@ struct payload {
>      unsigned int nsyms;                  /* Nr of entries in .strtab and 
> symbols. */
>      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
>      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> -    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. */

These both seem like they want a const in the middle.

> @@ -515,6 +520,27 @@ static int prepare_payload(struct payload *payload,
>          }
>      }
>  
> +    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");

Is the .hooks infix really needed?

> +    if ( sec )
> +    {
> +        if ( !sec->sec->sh_size ||

What's wrong with a zero size section (holding no hooks)? We've
had that same case elsewhere in the original series ...

> @@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
>  
>      arch_livepatch_quiesce();
>  
> +    /*
> +     * The hooks may call common code which expects spinlocks to be certain
> +     * type, as such disable this temporarily.
> +     */
> +    spin_debug_disable();
> +    for ( i = 0; i < data->n_load_funcs; i++ )
> +        data->load_funcs[i]();
> +    spin_debug_enable();

I have to admit that I can't really make sense of the comment, and
hence the code.

> --- /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)) \
> +        livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;

There's again a const desirable here, to render the section r/o.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

On Fri, Aug 05, 2016 at 09:35:49AM -0600, Jan Beulich wrote:
> >>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
> > In general, the hooks provide flexibility when having to deal with
> > unforeseen cases, but their application should be rarely required (<
> > 10%)."
> 
> But the greater flexibility of course comes with increased chances
> of screwing things up. I'm therefore still not entirely convinced that
> such XSAs wouldn't then better not be live patched.
> 
> > --- a/xen/arch/x86/test/xen_hello_world.c
> > +++ b/xen/arch/x86/test/xen_hello_world.c
> > @@ -4,14 +4,50 @@
> >   */
> >  
> >  #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)
> 
> And that Z_ prefix is supposed to guarantee that being last? Isn't
> it that upper case letters sort before lower case ones?

Yes. And the code is actually flexible enough not to be the last one.

B/c:
> 
> > +{
> > +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
> > +    BUG_ON(cnt == 0 || cnt > 2);
> > +    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 4! */
> 
> Isn't this an indication of a general weakness: Shouldn't a patch
> module be allowed to assume it's being run in a clean state, i.e.
> without any of its static data altered from their load time values?

Of this bug. (which I obviously need to fix).
> 
> > @@ -70,7 +71,11 @@ struct payload {
> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
> > symbols. */
> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> > -    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. */
> 
> These both seem like they want a const in the middle.

I did that initially and found out that the calling ->load_funcs[i] resulted
in _no_ code being called.

I did not dig in the assembler output to figure out what happend, let me
do that now.


> 
> > @@ -515,6 +520,27 @@ static int prepare_payload(struct payload *payload,
> >          }
> >      }
> >  
> > +    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
> 
> Is the .hooks infix really needed?
> 
> > +    if ( sec )
> > +    {
> > +        if ( !sec->sec->sh_size ||
> 
> What's wrong with a zero size section (holding no hooks)? We've
> had that same case elsewhere in the original series ...

That would be a bit odd (having zero size).

But yes there is nothing wrong with it.  I will fix it up.

> 
> > @@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
> >  
> >      arch_livepatch_quiesce();
> >  
> > +    /*
> > +     * The hooks may call common code which expects spinlocks to be certain
> > +     * type, as such disable this temporarily.
> > +     */
> > +    spin_debug_disable();
> > +    for ( i = 0; i < data->n_load_funcs; i++ )
> > +        data->load_funcs[i]();
> > +    spin_debug_enable();
> 
> I have to admit that I can't really make sense of the comment, and
> hence the code.

Perhaps:

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 affinity state"

?
> 
> > --- /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)) \
> > +        livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
> 
> There's again a const desirable here, to render the section r/o.

<nods> Let me dig in this. I can't recall when I initially tried
making the livepatch_loadcall_t be const whether I had this as const or not..

Thank you for your quick review!
> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 05.08.16 at 23:08, <konrad.wilk@oracle.com> wrote:
> On Fri, Aug 05, 2016 at 09:35:49AM -0600, Jan Beulich wrote:
>> >>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>> > In general, the hooks provide flexibility when having to deal with
>> > unforeseen cases, but their application should be rarely required (<
>> > 10%)."
>> 
>> But the greater flexibility of course comes with increased chances
>> of screwing things up. I'm therefore still not entirely convinced that
>> such XSAs wouldn't then better not be live patched.
>> 
>> > --- a/xen/arch/x86/test/xen_hello_world.c
>> > +++ b/xen/arch/x86/test/xen_hello_world.c
>> > @@ -4,14 +4,50 @@
>> >   */
>> >  
>> >  #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)
>> 
>> And that Z_ prefix is supposed to guarantee that being last? Isn't
>> it that upper case letters sort before lower case ones?
> 
> Yes. And the code is actually flexible enough not to be the last one.
> 
> B/c:
>> 
>> > +{
>> > +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
>> > +    BUG_ON(cnt == 0 || cnt > 2);
>> > +    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 4! */
>> 
>> Isn't this an indication of a general weakness: Shouldn't a patch
>> module be allowed to assume it's being run in a clean state, i.e.
>> without any of its static data altered from their load time values?
> 
> Of this bug. (which I obviously need to fix).

And the fix of which then should perhaps become a prereq to this
change.

>> > @@ -70,7 +71,11 @@ struct payload {
>> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
>> > symbols. */
>> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
>> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
>> > -    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. */
>> 
>> These both seem like they want a const in the middle.
> 
> I did that initially and found out that the calling ->load_funcs[i] resulted
> in _no_ code being called.
> 
> I did not dig in the assembler output to figure out what happend, let me
> do that now.

Indeed - I can't see how a const modifier here could alter whether
or not the pointed to function gets called.

>> > +    if ( sec )
>> > +    {
>> > +        if ( !sec->sec->sh_size ||
>> 
>> What's wrong with a zero size section (holding no hooks)? We've
>> had that same case elsewhere in the original series ...
> 
> That would be a bit odd (having zero size).

What's odd there? A simplistic generation tool could simply produce
all sections, no matter whether some of them have no contents.

>> > @@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
>> >  
>> >      arch_livepatch_quiesce();
>> >  
>> > +    /*
>> > +     * The hooks may call common code which expects spinlocks to be certain
>> > +     * type, as such disable this temporarily.
>> > +     */
>> > +    spin_debug_disable();
>> > +    for ( i = 0; i < data->n_load_funcs; i++ )
>> > +        data->load_funcs[i]();
>> > +    spin_debug_enable();
>> 
>> I have to admit that I can't really make sense of the comment, and
>> hence the code.
> 
> Perhaps:
> 
> 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 affinity state"

Apart from that last thing in quotes (I'd at least drop "affinity" and
perhaps make it "IRQ state checks", and then without quotes) this
sounds quite a bit better.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

> >> > @@ -70,7 +71,11 @@ struct payload {
> >> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
> >> > symbols. */
> >> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
> >> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> >> > -    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. */
> >> 
> >> These both seem like they want a const in the middle.
> > 
> > I did that initially and found out that the calling ->load_funcs[i] resulted
> > in _no_ code being called.
> > 
> > I did not dig in the assembler output to figure out what happend, let me
> > do that now.
> 
> Indeed - I can't see how a const modifier here could alter whether
> or not the pointed to function gets called.

I did a diff on the pre-assemble part (-S) with and without const on the above
structs. With 'const' we get:

        call    spin_debug_disable
 .LVL163:
-       .loc 1 1093 0
-       cmpl    $0, 324(%rbx)
-       je      .L112
-       movl    $0, %r12d
-.LVL164:
-.L113:
-       .loc 1 1094 0
-       mov     %r12d, %edx
-       movq    312(%rbx), %rax
-       call    *(%rax,%rdx,8)
-       .loc 1 1093 0
-       addl    $1, %r12d
-.LVL165:
-       cmpl    %r12d, 324(%rbx)
-       ja      .L113
-.LVL166:
-.L112:

being removed.

I thought it may have to do with the function not returning anything. But
even after making the typedef return it still omitted the call data->load_funcs[i]();


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 09.08.16 at 20:01, <konrad.wilk@oracle.com> wrote:
>> >> > @@ -70,7 +71,11 @@ struct payload {
>> >> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
>> >> > symbols. */
>> >> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
>> >> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
>> >> > -    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. */
>> >> 
>> >> These both seem like they want a const in the middle.
>> > 
>> > I did that initially and found out that the calling ->load_funcs[i] resulted
>> > in _no_ code being called.
>> > 
>> > I did not dig in the assembler output to figure out what happend, let me
>> > do that now.
>> 
>> Indeed - I can't see how a const modifier here could alter whether
>> or not the pointed to function gets called.
> 
> I did a diff on the pre-assemble part (-S) with and without const on the above
> structs. With 'const' we get:
> 
>         call    spin_debug_disable
>  .LVL163:
> -       .loc 1 1093 0
> -       cmpl    $0, 324(%rbx)
> -       je      .L112
> -       movl    $0, %r12d
> -.LVL164:
> -.L113:
> -       .loc 1 1094 0
> -       mov     %r12d, %edx
> -       movq    312(%rbx), %rax
> -       call    *(%rax,%rdx,8)
> -       .loc 1 1093 0
> -       addl    $1, %r12d
> -.LVL165:
> -       cmpl    %r12d, 324(%rbx)
> -       ja      .L113
> -.LVL166:
> -.L112:
> 
> being removed.
> 
> I thought it may have to do with the function not returning anything. But
> even after making the typedef return it still omitted the call 
> data->load_funcs[i]();

Odd. I've tried this simple example:

typedef int fn_t(void);

struct s {
	unsigned n;
	fn_t**fn;
	fn_t*const*fnc;
	const fn_t**cfn;
};

int test1(const struct s*ps) {
	unsigned i;
	int rc = 0;

	for(i = 0; !rc && i < ps->n; ++i)
		rc = ps->fn[i]();

	return rc;
}

int test2(const struct s*ps) {
	unsigned i;
	int rc = 0;

	for(i = 0; !rc && i < ps->n; ++i)
		rc = ps->fnc[i]();

	return rc;
}

int test3(const struct s*ps) {
	unsigned i;
	int rc = 0;

	for(i = 0; !rc && i < ps->n; ++i)
		rc = ps->cfn[i]();

	return rc;
}

test1() and test2() get compiled identically. test3(), using the field
with the misplaced const, oddly enough gets compiled slightly
differently (and without a warning despite one would seem
warranted), yet the call doesn't get omitted. If, however, I change
the return type of fn_t to void, the function body of test3() ends
up empty, which is a compiler bug afaict, but which also suggests
that you've tried the variant with the misplaced const.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

On Wed, Aug 10, 2016 at 03:46:49AM -0600, Jan Beulich wrote:
> >>> On 09.08.16 at 20:01, <konrad.wilk@oracle.com> wrote:
> >> >> > @@ -70,7 +71,11 @@ struct payload {
> >> >> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
> >> >> > symbols. */
> >> >> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
> >> >> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> >> >> > -    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. */
> >> >> 
> >> >> These both seem like they want a const in the middle.
                                                      ^^^^^^
                                                      Right there
.. snip..
> Odd. I've tried this simple example:
.. snip..
> test1() and test2() get compiled identically. test3(), using the field
> with the misplaced const, oddly enough gets compiled slightly
> differently (and without a warning despite one would seem
> warranted), yet the call doesn't get omitted. If, however, I change
> the return type of fn_t to void, the function body of test3() ends
> up empty, which is a compiler bug afaict, but which also suggests
> that you've tried the variant with the misplaced const.

<sigh>

If I had read your email more carefuly (see above) I would not have
wasted your time on this! Sorry about that!

This 'const' business is quite interesting.

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 10.08.16 at 11:46, <JBeulich@suse.com> wrote:
> Odd. I've tried this simple example:
> 
> typedef int fn_t(void);
> 
> struct s {
> 	unsigned n;
> 	fn_t**fn;
> 	fn_t*const*fnc;
> 	const fn_t**cfn;
> };
> 
> int test1(const struct s*ps) {
> 	unsigned i;
> 	int rc = 0;
> 
> 	for(i = 0; !rc && i < ps->n; ++i)
> 		rc = ps->fn[i]();
> 
> 	return rc;
> }
> 
> int test2(const struct s*ps) {
> 	unsigned i;
> 	int rc = 0;
> 
> 	for(i = 0; !rc && i < ps->n; ++i)
> 		rc = ps->fnc[i]();
> 
> 	return rc;
> }
> 
> int test3(const struct s*ps) {
> 	unsigned i;
> 	int rc = 0;
> 
> 	for(i = 0; !rc && i < ps->n; ++i)
> 		rc = ps->cfn[i]();
> 
> 	return rc;
> }
> 
> test1() and test2() get compiled identically. test3(), using the field
> with the misplaced const, oddly enough gets compiled slightly
> differently (and without a warning despite one would seem
> warranted), yet the call doesn't get omitted. If, however, I change
> the return type of fn_t to void, the function body of test3() ends
> up empty, which is a compiler bug afaict, but which also suggests
> that you've tried the variant with the misplaced const.

FTR: This is not a compiler bug, as specifically named undefined
in the C spec.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case"):
> On 10.08.16 at 11:46, <JBeulich@suse.com> wrote:
> > Odd. I've tried this simple example:
> > 
> > typedef int fn_t(void);
...
> > 	const fn_t**cfn;

Ie,
        const int **cfn(void);

> > 	for(i = 0; !rc && i < ps->n; ++i)
> > 		rc = ps->cfn[i]();

From `(gcc-4)Function Attributes':

  `const'
       Many functions do not examine any values except their arguments,
       and have no effects except the return value.  Basically this is
       just slightly more strict class than the `pure' attribute below,
       since function is not allowed to read global memory.

       Note that a function that has pointer arguments and examines the
       data pointed to must _not_ be declared `const'.  Likewise, a
       function that calls a non-`const' function usually must not be
       `const'.  It does not make sense for a `const' function to return
       `void'.

       The attribute `const' is not implemented in GCC versions earlier
       than 2.5.  An alternative way to declare that a function has no
       side effects, which works in the current version and in some older
       versions, is as follows:

            typedef int intfn ();

            extern const intfn square;

       This approach does not work in GNU C++ from 2.6.0 on, since the
       language specifies that the `const' must be attached to the return
       value.

Ie, gcc has always treated a function marked const as having no
unexpected inputs and no side effects.

> > test1() and test2() get compiled identically. test3(), using the field
> > with the misplaced const, oddly enough gets compiled slightly
> > differently (and without a warning despite one would seem
> > warranted), yet the call doesn't get omitted. If, however, I change
> > the return type of fn_t to void, the function body of test3() ends
> > up empty, which is a compiler bug afaict, but which also suggests
> > that you've tried the variant with the misplaced const.

No, it is gcc realising that there is no point calling a function
which returns void and has no side effects

TBH I am inclined to agree that gcc should issue a warning for a
such a function.

> FTR: This is not a compiler bug, as specifically named undefined
> in the C spec.

In this case, the behaviour is defined by the GCC manual.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 11.08.16 at 12:56, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/3] livepach: Add 
> .livepatch.hooks functions and test-case"):
>> On 10.08.16 at 11:46, <JBeulich@suse.com> wrote:
>> > Odd. I've tried this simple example:
>> > 
>> > typedef int fn_t(void);
> ...
>> > 	const fn_t**cfn;
> 
> Ie,
>         const int **cfn(void);
> 
>> > 	for(i = 0; !rc && i < ps->n; ++i)
>> > 		rc = ps->cfn[i]();
> 
> From `(gcc-4)Function Attributes':
> 
>   `const'
>        Many functions do not examine any values except their arguments,
>        and have no effects except the return value.  Basically this is
>        just slightly more strict class than the `pure' attribute below,
>        since function is not allowed to read global memory.
> 
>        Note that a function that has pointer arguments and examines the
>        data pointed to must _not_ be declared `const'.  Likewise, a
>        function that calls a non-`const' function usually must not be
>        `const'.  It does not make sense for a `const' function to return
>        `void'.
> 
>        The attribute `const' is not implemented in GCC versions earlier
>        than 2.5.  An alternative way to declare that a function has no
>        side effects, which works in the current version and in some older
>        versions, is as follows:
> 
>             typedef int intfn ();
> 
>             extern const intfn square;
> 
>        This approach does not work in GNU C++ from 2.6.0 on, since the
>        language specifies that the `const' must be attached to the return
>        value.
> 
> Ie, gcc has always treated a function marked const as having no
> unexpected inputs and no side effects.

Oh, I've always assumed that would be __attribute__((const))
only, but what you quote above proves me wrong.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[George Dunlap]

On 05/08/16 16:35, Jan Beulich wrote:
>>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>> In general, the hooks provide flexibility when having to deal with
>> unforeseen cases, but their application should be rarely required (<
>> 10%)."
> 
> But the greater flexibility of course comes with increased chances
> of screwing things up. I'm therefore still not entirely convinced that
> such XSAs wouldn't then better not be live patched.

But this functionality is optional, right?  So although I tend to agree
with you, we can let the person / organization preparing the patch take
that risk if they want to?

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 08.08.16 at 15:42, <george.dunlap@citrix.com> wrote:
> On 05/08/16 16:35, Jan Beulich wrote:
>>>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>>> In general, the hooks provide flexibility when having to deal with
>>> unforeseen cases, but their application should be rarely required (<
>>> 10%)."
>> 
>> But the greater flexibility of course comes with increased chances
>> of screwing things up. I'm therefore still not entirely convinced that
>> such XSAs wouldn't then better not be live patched.
> 
> But this functionality is optional, right?  So although I tend to agree
> with you, we can let the person / organization preparing the patch take
> that risk if they want to?

That's a valid point, albeit I think patch producer and patch consumer
might have different views, and the patch consumer may not know
(or even know to care) whether (s)he's got handed a patch with or
without use of hooks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[George Dunlap]

On 08/08/16 15:01, Jan Beulich wrote:
>>>> On 08.08.16 at 15:42, <george.dunlap@citrix.com> wrote:
>> On 05/08/16 16:35, Jan Beulich wrote:
>>>>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>>>> In general, the hooks provide flexibility when having to deal with
>>>> unforeseen cases, but their application should be rarely required (<
>>>> 10%)."
>>>
>>> But the greater flexibility of course comes with increased chances
>>> of screwing things up. I'm therefore still not entirely convinced that
>>> such XSAs wouldn't then better not be live patched.
>>
>> But this functionality is optional, right?  So although I tend to agree
>> with you, we can let the person / organization preparing the patch take
>> that risk if they want to?
> 
> That's a valid point, albeit I think patch producer and patch consumer
> might have different views, and the patch consumer may not know
> (or even know to care) whether (s)he's got handed a patch with or
> without use of hooks.

But the patch consumer probably doesn't know for sure if the patch was
even tested, or if it even fixes the things it was meant to fix.  The
producer / consumer relationship will always have an aspect of trust and
some mechanism for feedback.

I suppose there is a sort of "prisoner's dilemma" aspect to this though:
suppose provider A thinks that hooks are too dangerous, and doesn't
provide a hotfix for a certain XSA as a result, but provider B, seeing
an opportunity, ignores the risks and provides a hotpatch for their
product instead.  If the consumers can't evaluate the danger, provider A
will be pressured into providing a patch even though they don't think
it's a good idea.

So if this really is too dangerous to be used regularly, there is an
argument to be made not to accept the functionality.  (I'm not making an
argument either way at the moment.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Jan Beulich]

>>> On 08.08.16 at 16:10, <george.dunlap@citrix.com> wrote:
> On 08/08/16 15:01, Jan Beulich wrote:
>>>>> On 08.08.16 at 15:42, <george.dunlap@citrix.com> wrote:
>>> On 05/08/16 16:35, Jan Beulich wrote:
>>>>>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>>>>> In general, the hooks provide flexibility when having to deal with
>>>>> unforeseen cases, but their application should be rarely required (<
>>>>> 10%)."
>>>>
>>>> But the greater flexibility of course comes with increased chances
>>>> of screwing things up. I'm therefore still not entirely convinced that
>>>> such XSAs wouldn't then better not be live patched.
>>>
>>> But this functionality is optional, right?  So although I tend to agree
>>> with you, we can let the person / organization preparing the patch take
>>> that risk if they want to?
>> 
>> That's a valid point, albeit I think patch producer and patch consumer
>> might have different views, and the patch consumer may not know
>> (or even know to care) whether (s)he's got handed a patch with or
>> without use of hooks.
> 
> But the patch consumer probably doesn't know for sure if the patch was
> even tested, or if it even fixes the things it was meant to fix.  The
> producer / consumer relationship will always have an aspect of trust and
> some mechanism for feedback.
> 
> I suppose there is a sort of "prisoner's dilemma" aspect to this though:
> suppose provider A thinks that hooks are too dangerous, and doesn't
> provide a hotfix for a certain XSA as a result, but provider B, seeing
> an opportunity, ignores the risks and provides a hotpatch for their
> product instead.  If the consumers can't evaluate the danger, provider A
> will be pressured into providing a patch even though they don't think
> it's a good idea.
> 
> So if this really is too dangerous to be used regularly, there is an
> argument to be made not to accept the functionality.  (I'm not making an
> argument either way at the moment.)

Since both Konrad and Ross appear to be desiring hooks, and since
they're the maintainers, just me having a weak opinion in the other
direction to me means we ought to allow these. If other general
maintainers were considering this too dangerous too, the picture
might change.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

[Andrew Cooper]

On 08/08/16 16:15, Jan Beulich wrote:
>>>> On 08.08.16 at 16:10, <george.dunlap@citrix.com> wrote:
>> On 08/08/16 15:01, Jan Beulich wrote:
>>>>>> On 08.08.16 at 15:42, <george.dunlap@citrix.com> wrote:
>>>> On 05/08/16 16:35, Jan Beulich wrote:
>>>>>>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>>>>>> In general, the hooks provide flexibility when having to deal with
>>>>>> unforeseen cases, but their application should be rarely required (<
>>>>>> 10%)."
>>>>> But the greater flexibility of course comes with increased chances
>>>>> of screwing things up. I'm therefore still not entirely convinced that
>>>>> such XSAs wouldn't then better not be live patched.
>>>> But this functionality is optional, right?  So although I tend to agree
>>>> with you, we can let the person / organization preparing the patch take
>>>> that risk if they want to?
>>> That's a valid point, albeit I think patch producer and patch consumer
>>> might have different views, and the patch consumer may not know
>>> (or even know to care) whether (s)he's got handed a patch with or
>>> without use of hooks.
>> But the patch consumer probably doesn't know for sure if the patch was
>> even tested, or if it even fixes the things it was meant to fix.  The
>> producer / consumer relationship will always have an aspect of trust and
>> some mechanism for feedback.
>>
>> I suppose there is a sort of "prisoner's dilemma" aspect to this though:
>> suppose provider A thinks that hooks are too dangerous, and doesn't
>> provide a hotfix for a certain XSA as a result, but provider B, seeing
>> an opportunity, ignores the risks and provides a hotpatch for their
>> product instead.  If the consumers can't evaluate the danger, provider A
>> will be pressured into providing a patch even though they don't think
>> it's a good idea.
>>
>> So if this really is too dangerous to be used regularly, there is an
>> argument to be made not to accept the functionality.  (I'm not making an
>> argument either way at the moment.)
> Since both Konrad and Ross appear to be desiring hooks, and since
> they're the maintainers, just me having a weak opinion in the other
> direction to me means we ought to allow these. If other general
> maintainers were considering this too dangerous too, the picture
> might change.

I find it funny that we are arguing over the safety of hooks like these
as part of a system expressly designed to overwrite arbitrary bits of
hypervisor code and data.

This is already a "you had better know exactly what you are doing" kind
of area.

There are a number of XSA cases where one-time data modification needs
to happen if live patching were to occur.

+1 introduce the hooks.  The worst that happens is that someone has yet
way to make things go wrong, but the best that happens is that more
issues can be correctly livepatched.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/3] symbols: Generate an xen-sym.map

[Konrad Rzeszutek Wilk]

You could construct _most_ of the names of the functions
by doing 'nm --defined' but unfortunatly you do not get the
<file> prefix that is added on in Xen . For example:

$ cat xen-syms.symbols |grep do_domain_pause
0xffff82d080104920 t domain.c#do_domain_pause
$ nm --defined xen-syms|grep do_domain_pause
ffff82d080104920 t do_domain_pause

This is normally not an issue, but if one is doing livepatching and
wants during build-time verify that the symbols the livepatch payloads
will patch do correspond to the one the hypervisor has built - this helps a lot.

Note that during runtime one can do:
ffff82d080104920 t domain.c#do_domain_pause

But one may not want to build and verify a livepatch on the same host.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc:Ross Lagerwall <ross.lagerwall@citrix.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>
---
 .gitignore            |  1 +
 xen/Makefile          |  4 +++-
 xen/arch/arm/Makefile |  3 +++
 xen/arch/x86/Makefile |  4 ++++
 xen/tools/symbols.c   | 12 +++++++++++-
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 9b8dece..51c7283 100644
--- a/.gitignore
+++ b/.gitignore
@@ -286,6 +286,7 @@ tools/flask/policy/policy.conf
 tools/flask/policy/xenpolicy-*
 xen/xen
 xen/xen-syms
+xen/xen-syms.map
 xen/xen.*
 unmodified_drivers/linux-2.6/.tmp_versions
 unmodified_drivers/linux-2.6/*.cmd
diff --git a/xen/Makefile b/xen/Makefile
index ee8ce8e..3198d28 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -62,6 +62,7 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
 	ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z)
 	[ -d "$(D)$(DEBUG_DIR)" ] || $(INSTALL_DIR) $(D)$(DEBUG_DIR)
 	$(INSTALL_DATA) $(TARGET)-syms $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION)
+	$(INSTALL_DATA) $(TARGET)-syms.map $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
 	$(INSTALL_DATA) $(KCONFIG_CONFIG) $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).config
 	if [ -r $(TARGET).efi -a -n '$(EFI_DIR)' ]; then \
 		[ -d $(D)$(EFI_DIR) ] || $(INSTALL_DIR) $(D)$(EFI_DIR); \
@@ -91,6 +92,7 @@ _uninstall:
 	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z)
 	rm -f $(D)$(BOOT_DIR)/$(T)$(Z)
 	rm -f $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION)
+	rm -f $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_FULLVERSION).efi
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi
@@ -112,7 +114,7 @@ _clean: delete-unfresh-files
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) clean
 	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
 	find . \( -name "*.o" -o -name ".*.d" \) -exec rm -f {} \;
-	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET)-syms *~ core
+	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET)-syms $(TARGET)-syms.map *~ core
 	rm -f include/asm-*/asm-offsets.h
 	rm -f .banner
 
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 23aaf52..4311083 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -99,6 +99,9 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
 	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
 		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort \
+		>$(@D)/$(@F).map
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(@D)/.$(@F).1.o -o $@
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b18f033..10a7022 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -135,6 +135,10 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
 		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort --warn-dup \
 		>$(@D)/.$(@F).1.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort --warn-dup \
+		>$(@D)/$(@F).map
+	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(@D)/.$(@F).1.o -o $@
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 941fbe7..8c5842d 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -52,6 +52,7 @@ static unsigned int table_size, table_cnt;
 static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext, _eextratext;
 static int all_symbols = 0;
 static int sort_by_name = 0;
+static int map_only = 0;
 static char symbol_prefix_char = '\0';
 static enum { fmt_bsd, fmt_sysv } input_format;
 static int compare_name(const void *p1, const void *p2);
@@ -181,7 +182,7 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 		*sym++ = '#';
 	}
 	strcpy(sym, str);
-	if (sort_by_name) {
+	if (sort_by_name || map_only) {
 		s->orig_symbol = strdup(SYMBOL_NAME(s));
 		s->type = stype; /* As s->sym[0] ends mangled. */
 	}
@@ -307,6 +308,13 @@ static void write_src(void)
 	unsigned int *markers;
 	char buf[KSYM_NAME_LEN+1];
 
+	if (map_only) {
+		for (i = 0; i < table_cnt; i++)
+			printf("%#llx %c %s\n", table[i].addr, table[i].type,
+						table[i].orig_symbol);
+
+		return;
+	}
 	printf("#include <xen/config.h>\n");
 	printf("#include <asm/types.h>\n");
 	printf("#if BITS_PER_LONG == 64 && !defined(SYMBOLS_ORIGIN)\n");
@@ -609,6 +617,8 @@ int main(int argc, char **argv)
 				sort_by_name = 1;
 			else if (strcmp(argv[i], "--warn-dup") == 0)
 				warn_dup = true;
+			else if (strcmp(argv[i], "--xensyms") == 0)
+				map_only = true;
 			else
 				usage();
 		}
-- 
2.5.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] symbols: Generate an xen-sym.map

[Konrad Rzeszutek Wilk]

On August 4, 2016 11:49:23 AM EDT, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>You could construct _most_ of the names of the functions
>by doing 'nm --defined' but unfortunatly you do not get the
><file> prefix that is added on in Xen . For example:
>
>$ cat xen-syms.symbols |grep do_domain_pause
>0xffff82d080104920 t domain.c#do_domain_pause
>$ nm --defined xen-syms|grep do_domain_pause
>ffff82d080104920 t do_domain_pause
>
>This is normally not an issue, but if one is doing livepatching and
>wants during build-time verify that the symbols the livepatch payloads
>will patch do correspond to the one the hypervisor has built - this
>helps a lot.
>
>Note that during runtime one can do:


Odd. I remeber updating the commit to have:

#cat /proc/xen/xensyms | grep do_domain_pause
>ffff82d080104920 t domain.c#do_domain_pause
>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] symbols: Generate an xen-sym.map

[Jan Beulich]

>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
> You could construct _most_ of the names of the functions
> by doing 'nm --defined' but unfortunatly you do not get the
> <file> prefix that is added on in Xen . For example:
> 
> $ cat xen-syms.symbols |grep do_domain_pause
> 0xffff82d080104920 t domain.c#do_domain_pause
> $ nm --defined xen-syms|grep do_domain_pause
> ffff82d080104920 t do_domain_pause
> 
> This is normally not an issue, but if one is doing livepatching and
> wants during build-time verify that the symbols the livepatch payloads
> will patch do correspond to the one the hypervisor has built - this helps a 
> lot.
> 
> Note that during runtime one can do:
> ffff82d080104920 t domain.c#do_domain_pause
> 
> But one may not want to build and verify a livepatch on the same host.

Well, I'm not particularly happy about getting yet another file
produced, but so be it.

> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -99,6 +99,9 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>  	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
>  	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
>  		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S
> +	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
> +		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort \
> +		>$(@D)/$(@F).map
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
>  	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
>  	    $(@D)/.$(@F).1.o -o $@
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -135,6 +135,10 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>  	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
>  		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort --warn-dup \
>  		>$(@D)/.$(@F).1.S
> +	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
> +		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort --warn-dup \
> +		>$(@D)/$(@F).map
> +	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
>  	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
>  	    $(@D)/.$(@F).1.o -o $@

Why are you producing the maps from intermediate rather than
the final binaries?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/3] livepatch: Sync cache of build-id before using it first time.

[Konrad Rzeszutek Wilk]

We don't print at bootup time the build-id. The reason is
that xen_build_init and livepatch_init are both __initcall
type routines. This meant that when livepatch_init called
xen_build_id, it would return -ENODATA as build_id_len was
not setup yet (b/c xen_build_init would be called later).

We fix this by calling xen_build_init in livepatch_init which
allows us to print the build-id of the hypervisor.

We also keep xen_build_init as __initcall because build-id
can be built without livepatching being enabled (so
no livepatch_init being called).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/common/livepatch.c      | 1 +
 xen/common/version.c        | 6 +++++-
 xen/include/xen/livepatch.h | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index f6dbd51..88a79d8 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1597,6 +1597,7 @@ static int __init livepatch_init(void)
     const void *binary_id;
     unsigned int len;
 
+    xen_build_init();
     if ( !xen_build_id(&binary_id, &len) )
         printk(XENLOG_INFO LIVEPATCH ": build-id: %*phN\n", len, binary_id);
 
diff --git a/xen/common/version.c b/xen/common/version.c
index 0f96849..4114664 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -117,11 +117,15 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
     return 0;
 }
 
-static int __init xen_build_init(void)
+int __init xen_build_init(void)
 {
     const Elf_Note *n = __note_gnu_build_id_start;
     unsigned int sz;
 
+    /* We may have been called already. */
+    if ( build_id_len )
+        return 0;
+
     /* --build-id invoked with wrong parameters. */
     if ( __note_gnu_build_id_end <= &n[0] )
         return -ENODATA;
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index ed49843..cfc9601 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -44,7 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char *symname);
 bool_t is_patch(const void *addr);
 int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
                        const void **p, unsigned int *len);
-
+void xen_build_init(void);
 /* Arch hooks. */
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
-- 
2.5.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] livepatch: Sync cache of build-id before using it first time.

[Jan Beulich]

>>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -44,7 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char 
> *symname);
>  bool_t is_patch(const void *addr);
>  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>                         const void **p, unsigned int *len);
> -
> +void xen_build_init(void);
>  /* Arch hooks. */

Please don't ditch a blank line like this. But this is the wrong header
anyway, or else you'd have to make version.c include it (which would
again seem odd). And as I now realize that same aspect applies to
xen_build_id_check() too - this needs to move into xen/version.h.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] livepatch: Sync cache of build-id before using it first time.

[Konrad Rzeszutek Wilk]

On Fri, Aug 05, 2016 at 09:40:43AM -0600, Jan Beulich wrote:
> >>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/include/xen/livepatch.h
> > +++ b/xen/include/xen/livepatch.h
> > @@ -44,7 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char 
> > *symname);
> >  bool_t is_patch(const void *addr);
> >  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
> >                         const void **p, unsigned int *len);
> > -
> > +void xen_build_init(void);
> >  /* Arch hooks. */
> 
> Please don't ditch a blank line like this. But this is the wrong header
> anyway, or else you'd have to make version.c include it (which would
> again seem odd). And as I now realize that same aspect applies to
> xen_build_id_check() too - this needs to move into xen/version.h.

Back in April (feels like eons ago) with
"xsplice: Stacking build-id dependency checking."

I have this comment (on v7 of it) - https://lists.xen.org/archives/html/xen-devel/2016-04/msg01492.html:
" Moved xen_build_id_check definition to xsplice.h from version.h
        (and dropping the #include's in version.h)"

on that patch.

Digging in the archive, the reason is outlined in:
https://lists.xen.org/archives/html/xen-devel/2016-04/msg00407.html
where:

	 @@ -17,4 +17,7 @@ const char *xen_deny(void);
	>  #include <xen/types.h>
	>  int xen_build_id(const void **p, unsigned int *len);
	>  
	> +#include <xen/elfstructs.h>
	> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len);

	The #include is misplaced again, and I'm rather hesitant to see
	version.h gain this dependency. Couldn't this go into xen/elf.h?

And I somehow made it go in version.h not elf.h (I believe that was due
to the Elf_Note being a macro - that is dependent on ELF_SIZE - which
was not set in elf.h - and would cause compilation issues).

Anyhow if I move those two in version.h:

In file included from console.c:13:0:
/home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/include/xen/version.h:19:30: error: unknown type name ‘Elf_Note’
 int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
                              ^~~~~~~~
make[4]: *** [console.o] Error 1
make[3]: *** [char/built_in.o] Error 2
make[3]: *** Waiting for unfinished jobs....
In file included from kernel.c:10:0:
/home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/include/xen/version.h:19:30: error: unknown type name ‘Elf_Note’
 int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
                              ^~~~~~~~
make[3]: *** [kernel.o] Error 1
make[2]: *** [/home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/common/built_in.o] Err


I belive I have to include the #include <xen/elfstructs.h> to deal with Elf_Note macro.

P.S.
This is the patch (copy-n-paste):

diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index cfc9601..c297056 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -42,9 +42,6 @@ int livepatch_op(struct xen_sysctl_livepatch_op *);
 void check_for_livepatch_work(void);
 unsigned long livepatch_symbols_lookup_by_name(const char *symname);
 bool_t is_patch(const void *addr);
-int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
-                       const void **p, unsigned int *len);
-void xen_build_init(void);
 /* Arch hooks. */
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 400160f..19a3aaa 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -15,4 +15,10 @@ const char *xen_banner(void);
 const char *xen_deny(void);
 int xen_build_id(const void **p, unsigned int *len);
 
+#ifdef BUILD_ID
+int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
+                       const void **p, unsigned int *len);
+void xen_build_init(void);
+#endif
+
 #endif /* __XEN_VERSION_H__ */


> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] livepatch: Sync cache of build-id before using it first time.

[Jan Beulich]

>>> On 05.08.16 at 22:53, <konrad.wilk@oracle.com> wrote:
> On Fri, Aug 05, 2016 at 09:40:43AM -0600, Jan Beulich wrote:
>> >>> On 04.08.16 at 17:49, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/include/xen/livepatch.h
>> > +++ b/xen/include/xen/livepatch.h
>> > @@ -44,7 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char 
> 
>> > *symname);
>> >  bool_t is_patch(const void *addr);
>> >  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>> >                         const void **p, unsigned int *len);
>> > -
>> > +void xen_build_init(void);
>> >  /* Arch hooks. */
>> 
>> Please don't ditch a blank line like this. But this is the wrong header
>> anyway, or else you'd have to make version.c include it (which would
>> again seem odd). And as I now realize that same aspect applies to
>> xen_build_id_check() too - this needs to move into xen/version.h.
> 
> Back in April (feels like eons ago) with
> "xsplice: Stacking build-id dependency checking."
> 
> I have this comment (on v7 of it) - 
> https://lists.xen.org/archives/html/xen-devel/2016-04/msg01492.html: 
> " Moved xen_build_id_check definition to xsplice.h from version.h
>         (and dropping the #include's in version.h)"
> 
> on that patch.
> 
> Digging in the archive, the reason is outlined in:
> https://lists.xen.org/archives/html/xen-devel/2016-04/msg00407.html 
> where:
> 
> 	 @@ -17,4 +17,7 @@ const char *xen_deny(void);
> 	>  #include <xen/types.h>
> 	>  int xen_build_id(const void **p, unsigned int *len);
> 	>  
> 	> +#include <xen/elfstructs.h>
> 	> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len);
> 
> 	The #include is misplaced again, and I'm rather hesitant to see
> 	version.h gain this dependency. Couldn't this go into xen/elf.h?
> 
> And I somehow made it go in version.h not elf.h (I believe that was due
> to the Elf_Note being a macro - that is dependent on ELF_SIZE - which
> was not set in elf.h - and would cause compilation issues).
> 
> Anyhow if I move those two in version.h:
> 
> In file included from console.c:13:0:
> /home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/include/xen/version
> .h:19:30: error: unknown type name ‘Elf_Note’
>  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>                               ^~~~~~~~
> make[4]: *** [console.o] Error 1
> make[3]: *** [char/built_in.o] Error 2
> make[3]: *** Waiting for unfinished jobs....
> In file included from kernel.c:10:0:
> /home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/include/xen/version
> .h:19:30: error: unknown type name ‘Elf_Note’
>  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>                               ^~~~~~~~
> make[3]: *** [kernel.o] Error 1
> make[2]: *** 
> [/home/konrad/ssd/konrad/xtt-x86_64/bootstrap/xen.git/xen/common/built_in.o] 
> Err
> 
> 
> I belive I have to include the #include <xen/elfstructs.h> to deal with 
> Elf_Note macro.

That would be fine - that old comment of mine wasn't about dropping
the include, but moving it up in the file.

> P.S.
> This is the patch (copy-n-paste):

But this variant (which appears to get along without the #include) looks
fine too.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel