Re: [Xen-devel] [PATCH v8.1 12/27] xsplice: Implement support for applying/reverting/replacing patches.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:03 AM >>>
>--- a/docs/misc/xsplice.markdown
>+++ b/docs/misc/xsplice.markdown
>@@ -289,8 +289,13 @@ describing the functions to be patched:
 ><pre>
 >struct xsplice_patch_func {  
     >const char *name;  
>+#if BITS_PER_LONG == 32  
>+    uint32_t new_addr;  
>+    uint32_t old_addr;  
>+#else  
     >uint64_t new_addr;  
     >uint64_t old_addr;  
>+#endif
     
I don't think this belongs here (instead it should go wherever the structure 
gets
introduced).

>@@ -298,7 +303,8 @@ struct xsplice_patch_func {
 >};  
 ></pre>
 >
>-The size of the structure is 64 bytes.
>+The size of the structure is 64 bytes or 52 bytes if compiled under 32-bit
>+hypervisors.
 
And I'm uncertain this sentence is of much value.

>@@ -120,6 +121,7 @@ static void idle_loop(void)
         >(*pm_idle)();
         >do_tasklet();
         >do_softirq();
>+        check_for_xsplice_work(); /* Must be last. */
     
Without also saying why, the comment isn't very useful imo.

>+void arch_xsplice_apply_jmp(struct xsplice_patch_func_internal *func)
>+{
>+    int32_t val;
>+    uint8_t *old_ptr;
>+
>+    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->u.undo));
>+    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof val));

Standard sizeof() please.

>+    old_ptr = func->old_addr;
>+    memcpy(func->u.undo, 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);

Again (and I won't make further comments to that effect, the more that this was
requested in a previous round already anyway).

>+/*
>+ * Note that because of this NOP code the do_nmi is not safely patchable.
>+ * Also if we do receive 'real' NMIs we have lost them. Ditto for MCE.
>+ */

The reference to MCE is misleading here: You don't redirect the latter here, so 
its
handling is not the same as NMI.

>@@ -148,12 +224,12 @@ int arch_xsplice_secure(void *va, unsigned int pages, 
>enum va_type type)
     >ASSERT(va);
     >ASSERT(pages);
 >
>-    if ( type == XSPLICE_VA_RX ) /* PAGE_HYPERVISOR_RX */
>-        flag = _PAGE_PRESENT;
>-    else if ( type == XSPLICE_VA_RW ) /* PAGE_HYPERVISOR_RW */
>-        flag = _PAGE_RW | _PAGE_NX | _PAGE_PRESENT;
>-    else /* PAGE_HYPERVISOR_RO */
>-        flag = _PAGE_NX | _PAGE_PRESENT;
>+    if ( type == XSPLICE_VA_RX )
>+        flag = PAGE_HYPERVISOR_RX;
>+    else if ( type == XSPLICE_VA_RW )
>+        flag = PAGE_HYPERVISOR_RW;
>+    else
>+        flag = PAGE_HYPERVISOR_RO;
 
     Misplaced hunk (belongs in an earlier patch, and matches exactly what I 
had asked
for there).

>@@ -11,18 +12,28 @@
 >#include <xen/mm.h>
 >#include <xen/sched.h>
 >#include <xen/smp.h>
>+#include <xen/softirq.h>
 >#include <xen/spinlock.h>
 >#include <xen/vmap.h>
>+#include <xen/wait.h>
 >#include <xen/xsplice_elf.h>
 >#include <xen/xsplice.h>
 >
 >#include <asm/event.h>
 >#include <public/sysctl.h>
 >
>-/* Protects against payload_list operations. */
>+/*
>+ * Protects against payload_list operations and also allows only one
>+ * caller in schedule_work.
>+ */
 >static DEFINE_SPINLOCK(payload_lock);
 >static LIST_HEAD(payload_list);
 >
>+/*
>+ * Patches which have been applied.
>+ */

Ehem - comment style (still).

>+/* Defines an outstanding patching action. */
>+struct xsplice_work
>+{
>+    atomic_t semaphore;          /* Used for rendezvous. */
>+    atomic_t irq_semaphore;      /* Used to signal all IRQs disabled. */

Why do you, btw, need two of them? I would seem to me that having just one
for both purposes should be fine.

>+static int check_special_sections(const struct xsplice_elf *elf)
>+{
>+    unsigned int i;
>+    static const char *const names[] = { ".xsplice.funcs" };
>+    unsigned int count[ARRAY_SIZE(names)] = { 0 };
>+
>+    for ( i = 0; i < ARRAY_SIZE(names); i++ )
>+    {
>+        const struct xsplice_elf_sec *sec;
>+
>+        sec = xsplice_elf_sec_by_name(elf, names[i]);
>+        if ( !sec )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: %s is missing!\n",
>+                    elf->name, names[i]);
>+            return -EINVAL;
>+        }
>+
>+        if ( !sec->sec->sh_size )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: %s is empty!\n",
>+                    elf->name, names[i]);
>+            return -EINVAL;
>+        }
>+        count[i]++;

To conserve stack space, use a bool_t array instead and do the check-and-exit
here instead of in a second loop.

>+static int prepare_payload(struct payload *payload,
>+                           struct xsplice_elf *elf)
>+{
>+    const struct xsplice_elf_sec *sec;
>+    unsigned int i;
>+    struct xsplice_patch_func_internal *f;

Why is there a second ("internal") variant of this structure now? What
guarantees it to remain in sync with the non-"internal" one?

>+    sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs");

Since the string literal occurs at least twice, I'd suggest having a #define 
for it.

>+    ASSERT(sec);
>+    if ( sec->sec->sh_size % sizeof(*payload->funcs) )
>+    {
>+        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
>+                elf->name);
>+        return -EINVAL;
>+    }
>+
>+    payload->funcs = sec->load_addr;
>+    payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);

Following to our discussion yesterday - can't we (ab)use the section merge
flag here to report the structure size, along the lines of what relocation
sections do for their elements?

>+    for ( i = 0; i < payload->nfuncs; i++ )
>+    {
>+        int rc;
>+        unsigned int j;
>+
>+        f = &(payload->funcs[i]);
>+
>+        if ( f->version != XSPLICE_PAYLOAD_VERSION )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong version (%u). Expected 
>%d!\n",
>+                    elf->name, f->version, XSPLICE_PAYLOAD_VERSION);
>+            return -EOPNOTSUPP;
>+        }
>+
>+        if ( !f->new_addr || !f->new_size )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: Address or size fields are 
>zero!\n",
>+                    elf->name);
>+            return -EINVAL;
>+        }
>+
>+        rc = arch_xsplice_verify_func(f);
>+        if ( rc )
>+            return rc;
>+
>+        for ( j = 0; j < ARRAY_SIZE(f->u.pad); j++ )
>+            if ( f->u.pad[j] )
>+                return -EINVAL;
>+    }

Is that actually useful? It doesn't really matter what's in that space.

>+static int apply_payload(struct payload *data)
>+{
>+    unsigned int i;
>+
>+    printk(XENLOG_INFO XSPLICE "%s: Applying %u functions.\n",

No full stops in log messages please.

>+/*
>+ * This function is executed having all other CPUs with no stack (we may
>+ * have cpu_idle on it) and IRQs disabled. We guard against NMI by temporarily
>+ * installing our NOP NMI handler.
>+ */

The recurring (from the design doc) use of the wording "with no stack" has been
puzzling me from the beginning. Nothing really executes with no stack. How
about "without deep call stack" or some such?

Also - aren't we in common code here? If so, a reference to NMI doesn't belong
here (and a reference to MCE should really be added if the comment got moved
elsewhere).

>+static void xsplice_do_action(void)
>+{
>+    int rc;
>+    struct payload *data, *other, *tmp;
>+
>+    data = xsplice_work.data;
>+    /*
>+     * Now this function should be the only one on any stack.
>+     * No need to lock the payload list or applied list.
>+     */

Is "the only one" really a precise, architecture independent statement? Aren't
we getting here perhaps with at least one more return address on the stack
(the transition back from C to assembly code)?

>+static int xsplice_spin(atomic_t *counter, s_time_t timeout,
>+                           unsigned int cpus, const char *s)
>+{
>+    int rc = 0;
>+
>+    while ( atomic_read(counter) != cpus && NOW() < timeout )
>+        cpu_relax();
>+
>+    /* Log & abort. */
>+    if ( atomic_read(counter) != cpus )
>+    {
>+        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
>+               xsplice_work.data->name, s, atomic_read(counter), cpus);
>+        rc = -EBUSY;
>+        xsplice_work.data->rc = rc;
>+        xsplice_work.do_work = 0;
>+        smp_wmb();

Doesn't this need to move one line up? There's no further write following here,
so there's nothing this barrier separates right now.

>+            /*
>+             * Do NOT decrement semaphore down - as that may cause the other
>+             * CPU (which may be at this ready to increment it)

Odd wording?

>+             * to assume the role of master and then needlessly time out
>+             * out (as do_work is zero).
>+             */
>+            smp_wmb();

Same comment as above regarding placement.

>+            /* Flush the CPU i-cache via CPUID instruction (on x86). */
>+            arch_xsplice_post_action();

CPUID, as mentioned before, does not do any i-cache flushing.

>+ abort:
>+        per_cpu(work_to_do, cpu) = 0;
>+        xsplice_work.do_work = 0;
>+
>+        smp_wmb(); /* MUST complete writes before put_cpu_maps(). */
>+
>+        put_cpu_maps();

put_cpu_maps() is a spin unlock, i.e. a write barrier anyway.

>@@ -597,15 +1022,43 @@ static void xsplice_printall(unsigned char key)
     >}
 >
     >list_for_each_entry ( data, &payload_list, list )
>+    {
         >printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %zu 
pages.\n",
                >data->name, state2str(data->state), data->state, 
data->text_addr,
                >data->rw_addr, data->ro_addr, data->pages);
 >
>+        for ( i = 0; i < data->nfuncs; i++ )
>+        {
>+            struct xsplice_patch_func_internal *f = &(data->funcs[i]);
>+            printk("    %s patch %p(%u) with %p (%u)\n",
>+                   f->name, f->old_addr, f->old_size, f->new_addr, 
>f->new_size);
>+
>+            if ( i && !(i % 64) )
>+            {
>+                spin_unlock(&payload_lock);
>+                process_pending_softirqs();
>+                spin_lock(&payload_lock);

Iirc the initial acquire now intentionally is a try-lock. Hence it's quite clear
that this one can't be anything else either.

>static int __init xsplice_init(void)
 >{
>+    BUILD_BUG_ON( sizeof(struct xsplice_patch_func) !=
>+                  sizeof(struct xsplice_patch_func_internal) );
>+    BUILD_BUG_ON( sizeof(struct xsplice_patch_func_internal) !=
>+                  XSPLICE_PATCH_FUNC_INTERNAL_SIZE );
>+
>+#if BITS_PER_LONG == 64
>+    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_addr) != 8 );
>+    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_size) != 24 );
>+#else
>+    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_addr) != 4 );
>+    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_size) != 12 );
>+#endif
     
I don't see the point of these: If anything, you want to make sure "internal" 
and
normal structure offsets don't disagree.

>--- a/xen/include/asm-x86/current.h
>+++ b/xen/include/asm-x86/current.h
>@@ -86,10 +86,18 @@ static inline struct cpu_info *get_cpu_info(void)
 >unsigned long get_stack_trace_bottom(unsigned long sp);
 >unsigned long get_stack_dump_bottom (unsigned long sp);
 >
>+#ifdef CONFIG_XSPLICE
>+# define __CHECK_FOR_XSPLICE_WORK "call check_for_xsplice_work;"
>+#else
>+# define __CHECK_FOR_XSPLICE_WORK ""
>+#endif

No leading underscores please.

>--- a/xen/include/public/sysctl.h
>+++ b/xen/include/public/sysctl.h
>@@ -833,6 +833,33 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_featureset_t);
  >*     If zero exit with success.
  >*/
 >
>+#define XSPLICE_PAYLOAD_VERSION 1
>+/*
>+ * .xsplice.funcs structure layout defined in the `Payload format`
>+ * section in the xSplice design document.
>+ *
>+ * The size should be exactly 64 bytes (64) or 52 bytes (32).

Putting aside the usefulness of this comment, the items in parentheses aren't
really clear - DYM 64-bit and 32-bit respectively?

>+ * We guard this with __XEN__ as toolstacks do not need to use it.

I'd make this even more strict, if you want such a comment at all.

>+/*
>+ * The structure which defines the patching. This is what the hypervisor
>+ * expects in the '.xsplice.func' section of the ELF file.
>+ *
>+ * This MUST be in sync with what the tools generate. We expose
>+ * for the tools the 'struct xsplice_patch_func' which does not have
>+ * platform specific entries.
>+ */
>+#if BITS_PER_LONG == 64
>+#define XSPLICE_PATCH_FUNC_INTERNAL_SIZE    64
>+#else
>+#define XSPLICE_PATCH_FUNC_INTERNAL_SIZE    52
>+#endif

This is getting really odd. I can only hope all this goes away with ...

>+struct xsplice_patch_func_internal {

... this getting folded back into the public header version.

Jan


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