[PATCH v3] Livepatch fixes and features for v4.8.

[PATCH v3] Livepatch fixes and features for v4.8.

[Konrad Rzeszutek Wilk]

Hey!

This the posting of fixes and various left-overs that didn't get
quite done in 4.7 is marked as v3.


It should have been v2, but I messed up the emails previously
(cover letter said v1, patches said v2), so I figured we can start at v3.

Included are:
 - Bug-fixes
 - Parsing of symbol names encoded as: symbol+0x<offset>/<len>
 - NOP patching
 - Generating an symbol map file with Xen's unique symbols (file#symbol)
   so tools generating livepatch payloads can verify the right names.
 - Hooks

The 'hooks' are the most controversial part of this and are left as the
last patch :-)

In the v2 posting there was a bit of 'const' discussion going back and
forth and we learned that: "gcc has always treated a function marked const
as having no unexpected inputs and no side effects." meaning that:

 livepatch_loadcall_t **load_funcs;

is marked as:
 const livepatch_loadcall_t **load_funcs;
or
 livepatch_loadcall_t const **load_funcs;

Both end up omitting the call to the funcs and the hooks are not called.

Anyhow the git tree `

 git://enbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v3  

contains all the following patches:

 .gitignore                          |   1 +
 docs/misc/livepatch.markdown        |  31 ++++++-
 xen/Makefile                        |   4 +-
 xen/arch/arm/Makefile               |   3 +
 xen/arch/arm/livepatch.c            |   8 ++
 xen/arch/x86/Makefile               |   4 +
 xen/arch/x86/alternative.c          |   2 +-
 xen/arch/x86/livepatch.c            |  28 ++++--
 xen/arch/x86/test/xen_hello_world.c |  35 +++++++
 xen/common/livepatch.c              | 176 ++++++++++++++++++++++++++++++------
 xen/common/version.c                |   6 +-
 xen/include/asm-x86/alternative.h   |   1 +
 xen/include/xen/livepatch.h         |   3 +-
 xen/include/xen/livepatch_payload.h |  49 ++++++++++
 xen/include/xen/version.h           |   9 ++
 xen/tools/symbols.c                 |  12 ++-
 16 files changed, 325 insertions(+), 47 deletions(-)

Konrad Rzeszutek Wilk (8):
      livepatch: Clear .bss when payload is reverted
      livepatch: Deal with payloads without any .text
      version/livepatch: Move xen_build_id_check to version.h
      livepatch: Sync cache of build-id before using it first time.
      livepatch: Move code from prepare_payload to own routine
      livepatch: Add parsing for the symbol+0x<offset>/<len>
      livepatch: NOP if func->new_[addr,size] is zero.
      symbols: Generate an xen-sym.map

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 v3 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

So that when we apply the patch again the .bss is cleared.
Otherwise we may find some variables containing old values.

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

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v3: Initial submission
---
 xen/common/livepatch.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 5da28a3..47a23cf 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -70,6 +70,8 @@ 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). */
+    void *bss;                           /* .bss of the payload. */
+    size_t bss_size;                     /* and its size. */
     char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
 };
 
@@ -374,7 +376,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
                         elf->name, elf->sec[i].name, elf->sec[i].load_addr);
             }
             else
-                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
+            {
+                /* We expect only one BSS. */
+                if ( payload->bss )
+                {
+                    rc = -EINVAL;
+                    goto out;
+                }
+                payload->bss = elf->sec[i].load_addr;
+                payload->bss_size = elf->sec[i].sec->sh_size;
+
+                memset(payload->bss, 0, payload->bss_size);
+            }
         }
     }
 
@@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    /* And clear the BSS for subsequent operation. */
+    memset(data->bss, 0, data->bss_size);
+
     return 0;
 }
 
-- 
2.4.11


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

[PATCH v3 2/9] livepatch: Deal with payloads without any .text

[Konrad Rzeszutek Wilk]

It is possible. Especially if the only thing they do is
NOP functions - in which case there is only .livepatch.funcs
sections.

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

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission.
---
 xen/common/livepatch.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 47a23cf..17427b1 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -399,16 +399,17 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
 static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
 {
-    int rc;
+    int rc = 0;
     unsigned int text_pages, rw_pages, ro_pages;
 
     text_pages = PFN_UP(payload->text_size);
-    ASSERT(text_pages);
-
-    rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX);
-    if ( rc )
-        return rc;
 
+    if ( text_pages )
+    {
+        rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX);
+        if ( rc )
+            return rc;
+    }
     rw_pages = PFN_UP(payload->rw_size);
     if ( rw_pages )
     {
-- 
2.4.11


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

[PATCH v3 3/9] version/livepatch: Move xen_build_id_check to version.h

[Konrad Rzeszutek Wilk]

It makes more sense for it to be there. However that
means the version.h has now a dependency on <xen/elfstructs.h>
as the Elf_Note is a macro.

The elfstructs.h has a dependency on types.h as well so
we need that. We cannot put that #include <xen/types.h>
in elfstructs.h as that file is used by tools and they
do not have such file.

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

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
---
 xen/include/xen/livepatch.h | 2 --
 xen/include/xen/version.h   | 8 ++++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index ed49843..02f4572 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -42,8 +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);
 
 /* Arch hooks. */
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 400160f..97c247a 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -1,6 +1,9 @@
 #ifndef __XEN_VERSION_H__
 #define __XEN_VERSION_H__
 
+#include <xen/types.h>
+#include <xen/elfstructs.h>
+
 const char *xen_compile_date(void);
 const char *xen_compile_time(void);
 const char *xen_compile_by(void);
@@ -15,4 +18,9 @@ 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);
+#endif
+
 #endif /* __XEN_VERSION_H__ */
-- 
2.4.11


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

[PATCH v3 4/9] 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>

v2: Move xen_build_init in version.h instead of livepatch.h
---
 xen/common/livepatch.c    | 1 +
 xen/common/version.c      | 6 +++++-
 xen/include/xen/version.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 17427b1..28a400f 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1566,6 +1566,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/version.h b/xen/include/xen/version.h
index 97c247a..7a4b371 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -21,6 +21,7 @@ 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);
+int xen_build_init(void);
 #endif
 
 #endif /* __XEN_VERSION_H__ */
-- 
2.4.11


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

[PATCH v3 5/9] livepatch: Move code from prepare_payload to own routine

[Konrad Rzeszutek Wilk]

Specifically the code that is looking up f->old_addr - which
can be in its own routine instead of having it part of prepare_payload.

No functional change.

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

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v3: First submission.
---
 xen/common/livepatch.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 28a400f..cbfeac1 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -232,6 +232,29 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
     return n;
 }
 
+static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
+{
+    if ( f->old_addr )
+        return 0;
+
+    /* Lookup function's old address if not already resolved. */
+    f->old_addr = (void *)symbols_lookup_by_name(f->name);
+    if ( !f->old_addr )
+    {
+        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
+        if ( !f->old_addr )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
+                    elf->name, f->name);
+            return -ENOENT;
+        }
+    }
+    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
+            elf->name, f->name, f->old_addr);
+
+    return 0;
+}
+
 static struct payload *find_payload(const char *name)
 {
     struct payload *data, *found = NULL;
@@ -510,23 +533,9 @@ static int prepare_payload(struct payload *payload,
         if ( rc )
             return rc;
 
-        /* Lookup function's old address if not already resolved. */
-        if ( !f->old_addr )
-        {
-            f->old_addr = (void *)symbols_lookup_by_name(f->name);
-            if ( !f->old_addr )
-            {
-                f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
-                if ( !f->old_addr )
-                {
-                    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
-                            elf->name, f->name);
-                    return -ENOENT;
-                }
-            }
-            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
-                    elf->name, f->name, f->old_addr);
-        }
+        rc = lookup_symbol(f, elf);
+        if ( rc )
+            return rc;
     }
 
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
-- 
2.4.11


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

[PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

[Konrad Rzeszutek Wilk]

in case we want to patch at specific offsets inside
a function. (for example if we want to do NOP patching).

We also assume that the 'len' is only the size of
an isns that would be for a call opcode (so 5 bytes
on x86, and 4 on ARM 32/64).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
---
 docs/misc/livepatch.markdown |  2 +-
 xen/arch/arm/livepatch.c     |  8 ++++++++
 xen/arch/x86/livepatch.c     |  5 +++++
 xen/common/livepatch.c       | 43 +++++++++++++++++++++++++++++++++++++++++--
 xen/include/xen/livepatch.h  |  1 +
 5 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..f1a5147 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -312,7 +312,7 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 
 * `name` is the symbol name of the old function. Only used if `old_addr` is
    zero, otherwise will be used during dynamic linking (when hypervisor loads
-   the payload).
+   the payload). The format can be _symbol_ or _symbol+0x<offset>/<len>_.
 
 * `old_addr` is the address of the function to be patched and is filled in at
   payload generation time if hypervisor function address is known. If unknown,
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index aba1320..3093554 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -7,6 +7,14 @@
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
 
+/* On ARM32,64 instructions are always 4 bytes long. */
+#define PATCH_INSN_SIZE 4
+
+int arch_verify_insn_length(unsigned long len)
+{
+    return len != PATCH_INSN_SIZE;
+}
+
 void arch_livepatch_quiesce(void)
 {
 }
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 1023fab..df64b00 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -27,6 +27,11 @@ void arch_livepatch_revive(void)
     write_cr0(read_cr0() | X86_CR0_WP);
 }
 
+int arch_verify_insn_length(unsigned long len)
+{
+    return len != PATCH_INSN_SIZE;
+}
+
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
     /* No NOP patching yet. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index cbfeac1..e752949 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -234,14 +234,46 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
 
 static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
 {
+    const char *s;
+    char *plus = NULL, *slash = NULL;
+    unsigned long offset = 0;
+
     if ( f->old_addr )
         return 0;
 
+    s = f->name;
+    /* +<offset>/<len> */
+    plus = strchr(f->name, '+');
+    if ( plus )
+    {
+        slash = strchr(plus, '/');
+
+        if ( slash )
+        {
+            const char *endp = NULL;
+            unsigned int len;
+
+            offset = simple_strtoul(plus+1, &endp, 16);
+
+            if ( endp != slash )
+                return -EINVAL;
+
+            len = simple_strtoul(slash+1, NULL, 16);
+
+            if ( arch_verify_insn_length(len) )
+                return -EINVAL;
+
+            /* So that symbol lookup works. */
+            *plus = '\0';
+            s = f->name;
+        }
+    }
+
     /* Lookup function's old address if not already resolved. */
-    f->old_addr = (void *)symbols_lookup_by_name(f->name);
+    f->old_addr = (void *)symbols_lookup_by_name(s);
     if ( !f->old_addr )
     {
-        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
+        f->old_addr = (void *)livepatch_symbols_lookup_by_name(s);
         if ( !f->old_addr )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
@@ -249,6 +281,13 @@ static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
             return -ENOENT;
         }
     }
+
+    if ( plus && slash )
+    {
+        *plus = '+';
+        f->old_addr += offset;
+    }
+
     dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
             elf->name, f->name, f->old_addr);
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 02f4572..2e64686 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -44,6 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char *symname);
 bool_t is_patch(const void *addr);
 
 /* Arch hooks. */
+int arch_verify_insn_length(unsigned long len);
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
-- 
2.4.11


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

[PATCH v3 7/9] livepatch: NOP if func->new_[addr, size] is zero.

[Konrad Rzeszutek Wilk]

The NOP functionality will NOP any of the code at
the 'old_addr' or at 'name' if the 'new_addr' and 'new_size'
are both zero. The purpose of this is to NOP out calls, such as:

 e9 <4-bytes-offset>

(5 byte insn), or on ARM a 4 byte insn for branching.

We need the EIP of where we need to the NOP, and that can
be provided via the `old_addr` or `name`.

If the `old_addr` is provided we will NOP
5 instructions (on x86) at that location.

If `name` is provided with the symbol+0x<offset/<len>
we make sure that <len> is 5 (on x86) and upon retrieving the
EIP based on `name` will NOP that location.

While at it, also unify the code on x86 patching so
it is a bit simpler (instead of two seperate writes
just make it one memcpy).

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

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
---
 docs/misc/livepatch.markdown      |  6 ++++--
 xen/arch/x86/alternative.c        |  2 +-
 xen/arch/x86/livepatch.c          | 23 ++++++++++++++---------
 xen/common/livepatch.c            | 12 ++++++++----
 xen/include/asm-x86/alternative.h |  1 +
 5 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index f1a5147..590757d 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -320,10 +320,12 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 
 * `new_addr` is the address of the function that is replacing the old
   function. The address is filled in during relocation. The value **MUST** be
-  the address of the new function in the file.
+  either the address of the new function in the file, or zero if we are NOPing out
+  at `old_addr` (and `new_size` **MUST** be zero).
 
 * `old_size` and `new_size` contain the sizes of the respective functions in bytes.
-   The value of `old_size` **MUST** not be zero.
+   The value of `old_size` **MUST** not be zero. If the value of `new_size` is _zero_
+   the code at location of `old_addr` (or `name` when resolved) will be NOPed out.
 
 * `version` is to be one.
 
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index be40b13..fd8528e 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -101,7 +101,7 @@ static void __init arch_init_ideal_nops(void)
 }
 
 /* Use this to add nops to a buffer, then text_poke the whole buffer. */
-static void init_or_livepatch add_nops(void *insns, unsigned int len)
+void init_or_livepatch add_nops(void *insns, unsigned int len)
 {
     while ( len > 0 )
     {
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index df64b00..cabd0c1 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -34,10 +34,6 @@ int arch_verify_insn_length(unsigned long len)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
-    /* No NOP patching yet. */
-    if ( !func->new_size )
-        return -EOPNOTSUPP;
-
     if ( func->old_size < PATCH_INSN_SIZE )
         return -EINVAL;
 
@@ -46,18 +42,27 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
 
 void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
-    int32_t val;
     uint8_t *old_ptr;
+    uint8_t insn[PATCH_INSN_SIZE];
 
     BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
-    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
 
     old_ptr = func->old_addr;
     memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
 
-    *old_ptr++ = 0xe9; /* Relative jump */
-    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
-    memcpy(old_ptr, &val, sizeof(val));
+    if ( func->new_size )
+    {
+        int32_t val;
+
+        BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+
+        insn[0] = 0xe9;
+        val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
+        memcpy(&insn[1], &val, sizeof(val));
+    } else
+        add_nops(&insn, PATCH_INSN_SIZE);
+
+    memcpy(old_ptr, insn, PATCH_INSN_SIZE);
 }
 
 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index e752949..6f82a9e 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload,
             return -EOPNOTSUPP;
         }
 
-        if ( !f->new_addr || !f->new_size )
+        /* If both are zero then we are NOPing. */
+        if ( (!f->new_addr || !f->new_size) )
         {
-            dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n",
-                    elf->name);
-            return -EINVAL;
+            if ( f->new_addr || f->new_size )
+            {
+                dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n",
+                        elf->name);
+                return -EINVAL;
+            }
         }
 
         rc = arch_livepatch_verify_func(f);
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index bce959f..acaeded 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,7 @@ struct alt_instr {
     u8  replacementlen;     /* length of new instruction, <= instrlen */
 };
 
+extern void add_nops(void *insns, unsigned int len);
 /* Similar to apply_alternatives except it can be run with IRQs enabled. */
 extern void apply_alternatives_nocheck(struct alt_instr *start,
                                        struct alt_instr *end);
-- 
2.4.11


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

[PATCH v3 8/9] 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:
[root@localhost xen]# cat /proc/xen/xensyms |grep do_domain_pause
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>

v1: First submission
v2: Do not use intermediary object file but the final one
    Fix comment to have the example from /proc/xen/xensyms. Using '#'
    at start of line is a BAD IDEA.
---
 .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 d193820..44cc7bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -287,6 +287,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 76b60bc..4427771 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..0a96713 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -102,6 +102,9 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.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 $@
+	$(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort \
+		>$(@D)/$(@F).map
 	rm -f $(@D)/.$(@F).[0-9]*
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b18f033..d87ecd8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -136,8 +136,12 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort --warn-dup \
 		>$(@D)/.$(@F).1.S
 	$(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 $@
+	$(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort --warn-dup \
+		>$(@D)/$(@F).map
 	rm -f $(@D)/.$(@F).[0-9]*
 
 note.o: $(TARGET)-syms
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.4.11


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

[PATCH v3 9/9] 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>

---
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

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:
+ */
-- 
2.4.11


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

Re: [PATCH v3 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> @@ -374,7 +376,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>                          elf->name, elf->sec[i].name, elf->sec[i].load_addr);
>              }
>              else
> -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> +            {
> +                /* We expect only one BSS. */
> +                if ( payload->bss )
> +                {
> +                    rc = -EINVAL;

-EOPNOTSUPP according to e.g. the only- one-symbol-table code.

> +                    goto out;
> +                }
> +                payload->bss = elf->sec[i].load_addr;
> +                payload->bss_size = elf->sec[i].sec->sh_size;
> +
> +                memset(payload->bss, 0, payload->bss_size);
> +            }
>          }
>      }
>  
> @@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
>      list_del_rcu(&data->applied_list);
>      unregister_virtual_region(&data->region);
>  
> +    /* And clear the BSS for subsequent operation. */
> +    memset(data->bss, 0, data->bss_size);

Instead of doing it in two places, how about moving this into
apply_payload()?

Jan


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

Re: [PATCH v3 2/9] livepatch: Deal with payloads without any .text

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> It is possible. Especially if the only thing they do is
> NOP functions - in which case there is only .livepatch.funcs
> sections.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 3/9] version/livepatch: Move xen_build_id_check to version.h

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> It makes more sense for it to be there. However that
> means the version.h has now a dependency on <xen/elfstructs.h>
> as the Elf_Note is a macro.
> 
> The elfstructs.h has a dependency on types.h as well so
> we need that. We cannot put that #include <xen/types.h>
> in elfstructs.h as that file is used by tools and they
> do not have such file.

I think that's acceptable, as the number of places this header gets
included is pretty limited. Hence the alternative of making the
declaration conditional upon Elf_Note being defined is likely the
uglier one (as it would impose ordering constraints on the #include-s
used by respective source files).

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

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 4/9] livepatch: Sync cache of build-id before using it first time.

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> 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).

Now if the build ID is potentially useful outside of live patching,
shouldn't its logging also be done outside of livepatch code?

Jan


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

Re: [PATCH v3 5/9] livepatch: Move code from prepare_payload to own routine

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -232,6 +232,29 @@ static const char *livepatch_symbols_lookup(unsigned 
> long addr,
>      return n;
>  }
>  
> +static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)

The latter one can and hence should be const.

Jan


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

Re: [PATCH v3 4/9] livepatch: Sync cache of build-id before using it first time.

[Andrew Cooper]

On 15/08/16 11:38, Jan Beulich wrote:
>>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>> 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).
> Now if the build ID is potentially useful outside of live patching,
> shouldn't its logging also be done outside of livepatch code?

IMO the Build ID should be available and printed irrespective of live
patching.

It is useful information simply to identify the hypervisor.

~Andrew

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

Re: [PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> in case we want to patch at specific offsets inside
> a function. (for example if we want to do NOP patching).
> 
> We also assume that the 'len' is only the size of
> an isns that would be for a call opcode (so 5 bytes
> on x86, and 4 on ARM 32/64).

Which makes the notation using a slash quite confusing: Normally
if we see <symbol>+<offset>/<length> the length part is assumed
to be the overall size of the object/function the name refers to.
Could I talk you into using a different separator, like colon or
semicolon?

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -234,14 +234,46 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
>  
>  static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
>  {
> +    const char *s;
> +    char *plus = NULL, *slash = NULL;

Pointless initializers, afaict. Also the latter can be const.

> +    unsigned long offset = 0;
> +
>      if ( f->old_addr )
>          return 0;
>  
> +    s = f->name;
> +    /* +<offset>/<len> */
> +    plus = strchr(f->name, '+');
> +    if ( plus )
> +    {
> +        slash = strchr(plus, '/');
> +
> +        if ( slash )
> +        {
> +            const char *endp = NULL;
> +            unsigned int len;
> +
> +            offset = simple_strtoul(plus+1, &endp, 16);

Missing blanks around +.

> +            if ( endp != slash )
> +                return -EINVAL;
> +
> +            len = simple_strtoul(slash+1, NULL, 16);

Would you perhaps want to be even more precise and verify that
nothing follows this number?

Jan


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

Re: [PATCH v3 7/9] livepatch: NOP if func->new_[addr, size] is zero.

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' and 'new_size'
> are both zero. The purpose of this is to NOP out calls, such as:
> 
>  e9 <4-bytes-offset>

Except that E9 is JMP; CALL is E8.

> (5 byte insn), or on ARM a 4 byte insn for branching.
> 
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
> 
> If the `old_addr` is provided we will NOP
> 5 instructions (on x86) at that location.
> 
> If `name` is provided with the symbol+0x<offset/<len>
> we make sure that <len> is 5 (on x86) and upon retrieving the
> EIP based on `name` will NOP that location.

So why does this need to be restricted to 5-byte (on x86) code
blocks? I.e. what's wrong with NOP-ing out other code.

> @@ -46,18 +42,27 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
>  
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> -    int32_t val;
>      uint8_t *old_ptr;
> +    uint8_t insn[PATCH_INSN_SIZE];
>  
>      BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> -    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
>  
>      old_ptr = func->old_addr;
>      memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>  
> -    *old_ptr++ = 0xe9; /* Relative jump */
> -    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> -    memcpy(old_ptr, &val, sizeof(val));
> +    if ( func->new_size )
> +    {
> +        int32_t val;
> +
> +        BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> +        insn[0] = 0xe9;
> +        val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> +        memcpy(&insn[1], &val, sizeof(val));
> +    } else

Style.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload,
>              return -EOPNOTSUPP;
>          }
>  
> -        if ( !f->new_addr || !f->new_size )
> +        /* If both are zero then we are NOPing. */
> +        if ( (!f->new_addr || !f->new_size) )

Comment and condition contradict one another. And you're adding
unnecessary parentheses.

Jan


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

Re: [PATCH v3 8/9] symbols: Generate an xen-sym.map

[Jan Beulich]

>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -136,8 +136,12 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>  		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort --warn-dup \
>  		>$(@D)/.$(@F).1.S
>  	$(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 $@
> +	$(NM) -pa --format=sysv $(@D)/$(@F) \
> +		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort --warn-dup \
> +		>$(@D)/$(@F).map
>  	rm -f $(@D)/.$(@F).[0-9]*

So what about the xen.efi case? Is the map file there not of similar
relevance?

Also I don't think you want the --warn-dup here.

Jan


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

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

[Jan Beulich]

>>> 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


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

Re: [PATCH v3 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 04:27:38AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> > @@ -374,7 +376,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >                          elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> >              }
> >              else
> > -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > +            {
> > +                /* We expect only one BSS. */
> > +                if ( payload->bss )
> > +                {
> > +                    rc = -EINVAL;
> 
> -EOPNOTSUPP according to e.g. the only- one-symbol-table code.

Thanks!
> 
> > +                    goto out;
> > +                }
> > +                payload->bss = elf->sec[i].load_addr;
> > +                payload->bss_size = elf->sec[i].sec->sh_size;
> > +
> > +                memset(payload->bss, 0, payload->bss_size);
> > +            }
> >          }
> >      }
> >  
> > @@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
> >      list_del_rcu(&data->applied_list);
> >      unregister_virtual_region(&data->region);
> >  
> > +    /* And clear the BSS for subsequent operation. */
> > +    memset(data->bss, 0, data->bss_size);
> 
> Instead of doing it in two places, how about moving this into
> apply_payload()?

I was thinking of it too, but then it occurred to me that between the
'check' -> 'apply' the BSS is left unitialized. And if we ever crash
(right as somebody scheduled the livepatch) one could form the opinion
that it is due to the .bss having garbage. Or more importantly - the
hooks may not have run, but all its values looked like they have been
initialized.

And spend considerable amount of time debugging something which in reality
is not a problem?

Perhaps put it in multiple places :-)

> 
> Jan
> 

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

Re: [PATCH v3 4/9] livepatch: Sync cache of build-id before using it first time.

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 11:46:13AM +0100, Andrew Cooper wrote:
> On 15/08/16 11:38, Jan Beulich wrote:
> >>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> >> 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).
> > Now if the build ID is potentially useful outside of live patching,
> > shouldn't its logging also be done outside of livepatch code?

And by logging you mean the printk of it which is right now
only done in common/livepatch.c ?
> 
> IMO the Build ID should be available and printed irrespective of live
> patching.
> 
> It is useful information simply to identify the hypervisor.

Right, and it is.

As in you can build the hypervisor without livepatching and the build-id will be built in.

I can certainly make xen_build_id call:

 printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);

which would be a much simpler fix :-)

Let me do that.

> 
> ~Andrew

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

Re: [PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 04:53:48AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> > in case we want to patch at specific offsets inside
> > a function. (for example if we want to do NOP patching).
> > 
> > We also assume that the 'len' is only the size of
> > an isns that would be for a call opcode (so 5 bytes
> > on x86, and 4 on ARM 32/64).
> 
> Which makes the notation using a slash quite confusing: Normally
> if we see <symbol>+<offset>/<length> the length part is assumed
> to be the overall size of the object/function the name refers to.

Oh, I somehow had the length of instruction on my mind.

> Could I talk you into using a different separator, like colon or
> semicolon?

Of course. Whatever is easier to folks.

I can also ditch the <length> part altogether?

> 
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -234,14 +234,46 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
> >  
> >  static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
> >  {
> > +    const char *s;
> > +    char *plus = NULL, *slash = NULL;
> 
> Pointless initializers, afaict. Also the latter can be const.
> 
> > +    unsigned long offset = 0;
> > +
> >      if ( f->old_addr )
> >          return 0;
> >  
> > +    s = f->name;
> > +    /* +<offset>/<len> */
> > +    plus = strchr(f->name, '+');
> > +    if ( plus )
> > +    {
> > +        slash = strchr(plus, '/');
> > +
> > +        if ( slash )
> > +        {
> > +            const char *endp = NULL;
> > +            unsigned int len;
> > +
> > +            offset = simple_strtoul(plus+1, &endp, 16);
> 
> Missing blanks around +.
> 
> > +            if ( endp != slash )
> > +                return -EINVAL;
> > +
> > +            len = simple_strtoul(slash+1, NULL, 16);
> 
> Would you perhaps want to be even more precise and verify that
> nothing follows this number?

/me nods.
> 
> Jan
> 

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

Re: [PATCH v3 7/9] livepatch: NOP if func->new_[addr, size] is zero.

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 04:59:52AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> > The NOP functionality will NOP any of the code at
> > the 'old_addr' or at 'name' if the 'new_addr' and 'new_size'
> > are both zero. The purpose of this is to NOP out calls, such as:
> > 
> >  e9 <4-bytes-offset>
> 
> Except that E9 is JMP; CALL is E8.
> 
> > (5 byte insn), or on ARM a 4 byte insn for branching.
> > 
> > We need the EIP of where we need to the NOP, and that can
> > be provided via the `old_addr` or `name`.
> > 
> > If the `old_addr` is provided we will NOP
> > 5 instructions (on x86) at that location.
> > 
> > If `name` is provided with the symbol+0x<offset/<len>
> > we make sure that <len> is 5 (on x86) and upon retrieving the
> > EIP based on `name` will NOP that location.
> 
> So why does this need to be restricted to 5-byte (on x86) code
> blocks? I.e. what's wrong with NOP-ing out other code.

It can most certainly nop variable sizes. I will have to update
the design to make it clear that if 'new_addr' is zero then we will
NOP (and the .new_size will determine the amount of NOPs to sprinkle).

Let me do that along with your comments. Thanks!
> 
> > @@ -46,18 +42,27 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
> >  
> >  void arch_livepatch_apply_jmp(struct livepatch_func *func)
> >  {
> > -    int32_t val;
> >      uint8_t *old_ptr;
> > +    uint8_t insn[PATCH_INSN_SIZE];
> >  
> >      BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > -    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> >  
> >      old_ptr = func->old_addr;
> >      memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> >  
> > -    *old_ptr++ = 0xe9; /* Relative jump */
> > -    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> > -    memcpy(old_ptr, &val, sizeof(val));
> > +    if ( func->new_size )
> > +    {
> > +        int32_t val;
> > +
> > +        BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> > +
> > +        insn[0] = 0xe9;
> > +        val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> > +        memcpy(&insn[1], &val, sizeof(val));
> > +    } else
> 
> Style.
> 
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload,
> >              return -EOPNOTSUPP;
> >          }
> >  
> > -        if ( !f->new_addr || !f->new_size )
> > +        /* If both are zero then we are NOPing. */
> > +        if ( (!f->new_addr || !f->new_size) )
> 
> Comment and condition contradict one another. And you're adding
> unnecessary parentheses.
> 
> Jan
> 

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

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

[Konrad Rzeszutek Wilk]

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
> 

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

Re: [PATCH v3 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 15.08.16 at 16:29, <konrad.wilk@oracle.com> wrote:
> On Mon, Aug 15, 2016 at 04:27:38AM -0600, Jan Beulich wrote:
>> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>> > @@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
>> >      list_del_rcu(&data->applied_list);
>> >      unregister_virtual_region(&data->region);
>> >  
>> > +    /* And clear the BSS for subsequent operation. */
>> > +    memset(data->bss, 0, data->bss_size);
>> 
>> Instead of doing it in two places, how about moving this into
>> apply_payload()?
> 
> I was thinking of it too, but then it occurred to me that between the
> 'check' -> 'apply' the BSS is left unitialized. And if we ever crash
> (right as somebody scheduled the livepatch) one could form the opinion
> that it is due to the .bss having garbage. Or more importantly - the
> hooks may not have run, but all its values looked like they have been
> initialized.
> 
> And spend considerable amount of time debugging something which in reality
> is not a problem?
> 
> Perhaps put it in multiple places :-)

Well, you and Ross know. Afaic I'd always do things in just one
place if that's possible.

Jan


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

Re: [PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

[Jan Beulich]

>>> On 15.08.16 at 16:35, <konrad.wilk@oracle.com> wrote:
> On Mon, Aug 15, 2016 at 04:53:48AM -0600, Jan Beulich wrote:
>> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>> > in case we want to patch at specific offsets inside
>> > a function. (for example if we want to do NOP patching).
>> > 
>> > We also assume that the 'len' is only the size of
>> > an isns that would be for a call opcode (so 5 bytes
>> > on x86, and 4 on ARM 32/64).
>> 
>> Which makes the notation using a slash quite confusing: Normally
>> if we see <symbol>+<offset>/<length> the length part is assumed
>> to be the overall size of the object/function the name refers to.
> 
> Oh, I somehow had the length of instruction on my mind.
> 
>> Could I talk you into using a different separator, like colon or
>> semicolon?
> 
> Of course. Whatever is easier to folks.
> 
> I can also ditch the <length> part altogether?

The question is why you added it in the first place (see also the
comments on the other patch, where I'm puzzled by the restriction
to 5 bytes length).

Jan


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

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

[Jan Beulich]

>>> 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


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

Re: [PATCH v3 6/9] livepatch: Add parsing for the symbol+0x<offset>/<len>

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 09:12:21AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 16:35, <konrad.wilk@oracle.com> wrote:
> > On Mon, Aug 15, 2016 at 04:53:48AM -0600, Jan Beulich wrote:
> >> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
> >> > in case we want to patch at specific offsets inside
> >> > a function. (for example if we want to do NOP patching).
> >> > 
> >> > We also assume that the 'len' is only the size of
> >> > an isns that would be for a call opcode (so 5 bytes
> >> > on x86, and 4 on ARM 32/64).
> >> 
> >> Which makes the notation using a slash quite confusing: Normally
> >> if we see <symbol>+<offset>/<length> the length part is assumed
> >> to be the overall size of the object/function the name refers to.
> > 
> > Oh, I somehow had the length of instruction on my mind.
> > 
> >> Could I talk you into using a different separator, like colon or
> >> semicolon?
> > 
> > Of course. Whatever is easier to folks.
> > 
> > I can also ditch the <length> part altogether?
> 
> The question is why you added it in the first place (see also the
> comments on the other patch, where I'm puzzled by the restriction
> to 5 bytes length).

Oh, I was looking at the dump stacks and got in my mind that /<len>
MUST be part of it. But it most certainly does not and I can remove it.

> 
> Jan
> 

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

Re: [PATCH v3 1/9] livepatch: Clear .bss when payload is reverted

[Ross Lagerwall]

On 08/15/2016 04:10 PM, Jan Beulich wrote:
>>>> On 15.08.16 at 16:29, <konrad.wilk@oracle.com> wrote:
>> On Mon, Aug 15, 2016 at 04:27:38AM -0600, Jan Beulich wrote:
>>>>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>>>> @@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
>>>>      list_del_rcu(&data->applied_list);
>>>>      unregister_virtual_region(&data->region);
>>>>
>>>> +    /* And clear the BSS for subsequent operation. */
>>>> +    memset(data->bss, 0, data->bss_size);
>>>
>>> Instead of doing it in two places, how about moving this into
>>> apply_payload()?
>>
>> I was thinking of it too, but then it occurred to me that between the
>> 'check' -> 'apply' the BSS is left unitialized. And if we ever crash
>> (right as somebody scheduled the livepatch) one could form the opinion
>> that it is due to the .bss having garbage. Or more importantly - the
>> hooks may not have run, but all its values looked like they have been
>> initialized.
>>
>> And spend considerable amount of time debugging something which in reality
>> is not a problem?
>>
>> Perhaps put it in multiple places :-)
>
> Well, you and Ross know. Afaic I'd always do things in just one
> place if that's possible.
>

Yes, I'd also prefer it in just a single place.

However, assuming that there is only a single BSS section is wrong. 
Especially when compiling with -fdata-sections (as livepatch-build-tools 
does), a payload can have multiple BSS sections.

-- 
Ross Lagerwall

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

Re: [PATCH v3 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 19.08.16 at 10:37, <ross.lagerwall@citrix.com> wrote:
> On 08/15/2016 04:10 PM, Jan Beulich wrote:
>>>>> On 15.08.16 at 16:29, <konrad.wilk@oracle.com> wrote:
>>> On Mon, Aug 15, 2016 at 04:27:38AM -0600, Jan Beulich wrote:
>>>>>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote:
>>>>> @@ -1034,6 +1047,9 @@ static int revert_payload(struct payload *data)
>>>>>      list_del_rcu(&data->applied_list);
>>>>>      unregister_virtual_region(&data->region);
>>>>>
>>>>> +    /* And clear the BSS for subsequent operation. */
>>>>> +    memset(data->bss, 0, data->bss_size);
>>>>
>>>> Instead of doing it in two places, how about moving this into
>>>> apply_payload()?
>>>
>>> I was thinking of it too, but then it occurred to me that between the
>>> 'check' -> 'apply' the BSS is left unitialized. And if we ever crash
>>> (right as somebody scheduled the livepatch) one could form the opinion
>>> that it is due to the .bss having garbage. Or more importantly - the
>>> hooks may not have run, but all its values looked like they have been
>>> initialized.
>>>
>>> And spend considerable amount of time debugging something which in reality
>>> is not a problem?
>>>
>>> Perhaps put it in multiple places :-)
>>
>> Well, you and Ross know. Afaic I'd always do things in just one
>> place if that's possible.
>>
> 
> Yes, I'd also prefer it in just a single place.
> 
> However, assuming that there is only a single BSS section is wrong. 
> Especially when compiling with -fdata-sections (as livepatch-build-tools 
> does), a payload can have multiple BSS sections.

Good point, and making it an even stronger argument to deal with
that in just one place.

Jan


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

Re: [PATCH v3 3/9] version/livepatch: Move xen_build_id_check to version.h

[Ross Lagerwall]

On 08/14/2016 10:52 PM, Konrad Rzeszutek Wilk wrote:
> It makes more sense for it to be there. However that
> means the version.h has now a dependency on <xen/elfstructs.h>
> as the Elf_Note is a macro.
>
> The elfstructs.h has a dependency on types.h as well so
> we need that. We cannot put that #include <xen/types.h>
> in elfstructs.h as that file is used by tools and they
> do not have such file.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v3 2/9] livepatch: Deal with payloads without any .text

[Ross Lagerwall]

On 08/14/2016 10:52 PM, Konrad Rzeszutek Wilk wrote:
> It is possible. Especially if the only thing they do is
> NOP functions - in which case there is only .livepatch.funcs
> sections.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v3 5/9] livepatch: Move code from prepare_payload to own routine

[Ross Lagerwall]

On 08/14/2016 10:52 PM, Konrad Rzeszutek Wilk wrote:
> Specifically the code that is looking up f->old_addr - which
> can be in its own routine instead of having it part of prepare_payload.
>
> No functional change.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> v3: First submission.
> ---
>  xen/common/livepatch.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 28a400f..cbfeac1 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -232,6 +232,29 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
>      return n;
>  }
>
> +static int lookup_symbol(struct livepatch_func *f, struct livepatch_elf *elf)
> +{
> +    if ( f->old_addr )
> +        return 0;
> +
> +    /* Lookup function's old address if not already resolved. */

This comment needs to move further up for it to be correct, since at 
this point we know that it is not yet resolved. How about putting it at 
the top of the function?

> +    f->old_addr = (void *)symbols_lookup_by_name(f->name);
> +    if ( !f->old_addr )
> +    {
> +        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
> +        if ( !f->old_addr )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
> +                    elf->name, f->name);
> +            return -ENOENT;
> +        }
> +    }
> +    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
> +            elf->name, f->name, f->old_addr);
> +
> +    return 0;
> +}
> +
>  static struct payload *find_payload(const char *name)
>  {
>      struct payload *data, *found = NULL;
> @@ -510,23 +533,9 @@ static int prepare_payload(struct payload *payload,
>          if ( rc )
>              return rc;
>
> -        /* Lookup function's old address if not already resolved. */
> -        if ( !f->old_addr )
> -        {
> -            f->old_addr = (void *)symbols_lookup_by_name(f->name);
> -            if ( !f->old_addr )
> -            {
> -                f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
> -                if ( !f->old_addr )
> -                {
> -                    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
> -                            elf->name, f->name);
> -                    return -ENOENT;
> -                }
> -            }
> -            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
> -                    elf->name, f->name, f->old_addr);
> -        }
> +        rc = lookup_symbol(f, elf);
> +        if ( rc )
> +            return rc;

Can you give the function a less generic name? E.g. resolve_old_address

-- 
Ross Lagerwall

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