[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v1 08/11] xsplice: Implement support for applying patches



On 11/05/2015 03:17 AM, Konrad Rzeszutek Wilk wrote:
snip
diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
index dbff0d5..31e4124 100644
--- a/xen/arch/x86/xsplice.c
+++ b/xen/arch/x86/xsplice.c
@@ -3,6 +3,25 @@
  #include <xen/xsplice_elf.h>
  #include <xen/xsplice.h>

+#define PATCH_INSN_SIZE 5
+
+void xsplice_apply_jmp(struct xsplice_patch_func *func)

Don't we want for it to be 'int'

Only if an error is expected.

+{
+    uint32_t val;
+    uint8_t *old_ptr;
+
+    old_ptr = (uint8_t *)func->old_addr;
+    memcpy(func->undo, old_ptr, PATCH_INSN_SIZE);

And perhaps use something which can catch an exception (#GP) so that
this can error out?

Why would this fail?

+    *old_ptr++ = 0xe9; /* Relative jump */
+    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
+    memcpy(old_ptr, &val, sizeof val);
+}
+
+void xsplice_revert_jmp(struct xsplice_patch_func *func)
+{
+    memcpy((void *)func->old_addr, func->undo, PATCH_INSN_SIZE);
+}
+
  int xsplice_verify_elf(uint8_t *data, ssize_t len)
  {

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 5e88c55..4476be5 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -11,16 +11,21 @@
  #include <xen/guest_access.h>
  #include <xen/stdbool.h>
  #include <xen/sched.h>
+#include <xen/softirq.h>
  #include <xen/lib.h>
+#include <xen/wait.h>
  #include <xen/xsplice_elf.h>
  #include <xen/xsplice.h>
  #include <public/sysctl.h>

  #include <asm/event.h>
+#include <asm/nmi.h>

  static DEFINE_SPINLOCK(payload_list_lock);
  static LIST_HEAD(payload_list);

+static LIST_HEAD(applied_list);
+
  static unsigned int payload_cnt;
  static unsigned int payload_version = 1;

@@ -29,15 +34,34 @@ struct payload {
      int32_t rc;         /* 0 or -EXX. */

      struct list_head   list;   /* Linked to 'payload_list'. */
+    struct list_head   applied_list;   /* Linked to 'applied_list'. */

+    struct xsplice_patch_func *funcs;
+    int nfuncs;

unsigned int;

      void *module_address;
      size_t module_pages;

      char  id[XEN_XSPLICE_NAME_SIZE + 1];          /* Name of it. */
  };

+/* Defines an outstanding patching action. */
+struct xsplice_work
+{
+    atomic_t semaphore;          /* Used for rendezvous */
+    atomic_t irq_semaphore;      /* Used to signal all IRQs disabled */
+    struct payload *data;        /* The payload on which to act */
+    volatile bool_t do_work;     /* Signals work to do */
+    volatile bool_t ready;       /* Signals all CPUs synchronized */
+    uint32_t cmd;                /* Action request. XSPLICE_ACTION_* */

Now since you have a pointer to 'data' can't you follow that for the
cmd? Or at least the 'data->state'?

I moved cmd out of the payload and into xsplice_work since cmd is only needed when there is work to do. data->state contains the current state of the payload (i.e. before the action has been performed) so it provides no indication of what command needs to be performed.


Missing full stops.
+};
+
+static DEFINE_SPINLOCK(xsplice_work_lock);
+/* There can be only one outstanding patching action. */
+static struct xsplice_work xsplice_work;
+
  static int load_module(struct payload *payload, uint8_t *raw, ssize_t len);
  static void free_module(struct payload *payload);
+static int schedule_work(struct payload *data, uint32_t cmd);

  static const char *state2str(int32_t state)
  {
@@ -341,28 +365,22 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
      case XSPLICE_ACTION_REVERT:
          if ( data->state == XSPLICE_STATE_APPLIED )
          {
-            /* No implementation yet. */
-            data->state = XSPLICE_STATE_CHECKED;
-            data->rc = 0;
-            rc = 0;
+            data->rc = -EAGAIN;
+            rc = schedule_work(data, action->cmd);
          }
          break;
      case XSPLICE_ACTION_APPLY:
          if ( (data->state == XSPLICE_STATE_CHECKED) )
          {
-            /* No implementation yet. */
-            data->state = XSPLICE_STATE_APPLIED;
-            data->rc = 0;
-            rc = 0;
+            data->rc = -EAGAIN;
+            rc = schedule_work(data, action->cmd);
          }
          break;
      case XSPLICE_ACTION_REPLACE:
          if ( data->state == XSPLICE_STATE_CHECKED )
          {
-            /* No implementation yet. */
-            data->state = XSPLICE_STATE_CHECKED;
-            data->rc = 0;
-            rc = 0;
+            data->rc = -EAGAIN;
+            rc = schedule_work(data, action->cmd);
          }
          break;
      default:
@@ -637,6 +655,24 @@ static int perform_relocs(struct xsplice_elf *elf)
      return 0;
  }

+static int find_special_sections(struct payload *payload,
+                                 struct xsplice_elf *elf)
+{
+    struct xsplice_elf_sec *sec;
+
+    sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs");
+    if ( !sec )
+    {
+        printk(XENLOG_ERR ".xsplice.funcs is missing\n");
+        return -1;
+    }
+
+    payload->funcs = (struct xsplice_patch_func *)sec->load_addr;
+    payload->nfuncs = sec->sec->sh_size / (sizeof *payload->funcs);
+
+    return 0;
+}

That looks like it should belong to another patch?

Why? The array of functions is specifically needed for applying a payload so the code belongs in this patch.

+
  static int load_module(struct payload *payload, uint8_t *raw, ssize_t len)
  {
      struct xsplice_elf elf;
@@ -662,6 +698,10 @@ static int load_module(struct payload *payload, uint8_t 
*raw, ssize_t len)
      if ( rc )
          goto err_module;

+    rc = find_special_sections(payload, &elf);
+    if ( rc )
+        goto err_module;
+

Ditto?
      return 0;

   err_module:
@@ -672,6 +712,206 @@ static int load_module(struct payload *payload, uint8_t 
*raw, ssize_t len)
      return rc;
  }

+
+/*
+ * The following functions get the CPUs into an appropriate state and
+ * apply (or revert) each of the module's functions.

s/module/payload/

+ */
+
+/*
+ * This function is executed having all other CPUs with no stack and IRQs
+ * disabled.

Well, there is some stack. For example from 'cpu_idle' - you have the
'cpu_idle' on the stack.

+ */
+static int apply_payload(struct payload *data)
+{
+    int i;

unsigned int
+
+    printk(XENLOG_DEBUG "Applying payload: %s\n", data->id);
+
+    for ( i = 0; i < data->nfuncs; i++ )
+        xsplice_apply_jmp(data->funcs + i);

And if this returns an error then we could skip adding
it to the applied_list..

Only if an error is expected.

+

Also the patching in Linux seems to do some icache purging.
Should we use that?

I didn't see that when I looked for it. The alternatives patching in Xen doesn't purge the icache (afaict). We need feedback from an x86 maintainer here.


+    list_add_tail(&data->applied_list, &applied_list);
+
+    return 0;
+}
+
+/*
+ * This function is executed having all other CPUs with no stack and IRQs
+ * disabled.
+ */
+static int revert_payload(struct payload *data)
+{
+    int i;

unsigned int i;
+
+    printk(XENLOG_DEBUG "Reverting payload: %s\n", data->id);
+
+    for ( i = 0; i < data->nfuncs; i++ )
+        xsplice_revert_jmp(data->funcs + i);
+
+    list_del(&data->applied_list);
+
+    return 0;
+}
+
+/* Must be holding the payload_list lock */

Missing full stop.

Should that lock be called something else now? (Because it is certainly
not protecting the list anymore - but also the scheduling action).

Hmm...

+static int schedule_work(struct payload *data, uint32_t cmd)
+{
+    /* Fail if an operation is already scheduled */
+    if ( xsplice_work.do_work )
+        return -EAGAIN;
+

+    xsplice_work.cmd = cmd;
+    xsplice_work.data = data;
+    atomic_set(&xsplice_work.semaphore, 0);
+    atomic_set(&xsplice_work.irq_semaphore, 0);
+    xsplice_work.ready = false;
+    smp_mb();
+    xsplice_work.do_work = true;
+    smp_mb();

So this is your 'GO GO' signal right? I think you may want
to have 'smb_wmb()'

A full review of the memory barriers and synchronization is needed by someone more knowledgeable than me.

+
+    return 0;
+}
+

/me laughs. What a way to 'fix' the NMI watchdog.

It comes directly from the alternatives code.


+static int mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
+{
+    return 1;
+}
+
+static void reschedule_fn(void *unused)
+{
+    smp_mb(); /* Synchronize with setting do_work */
+    raise_softirq(SCHEDULE_SOFTIRQ);
+}
+
+/*
+ * The main function which manages the work of quiescing the system and
+ * patching code.
+ */
+void do_xsplice(void)
+{
+    int id;

unsigned int id;
+    unsigned int total_cpus;
+    nmi_callback_t saved_nmi_callback;
+
+    /* Fast path: no work to do */

Missing full stop.
+    if ( likely(!xsplice_work.do_work) )
+        return;
+
+    ASSERT(local_irq_is_enabled());
+
+    spin_lock(&xsplice_work_lock);
+    id = atomic_read(&xsplice_work.semaphore);
+    atomic_inc(&xsplice_work.semaphore);
+    spin_unlock(&xsplice_work_lock);

Could you use 'atomic_inc_and_test' and then you can get
rid of the spinlock.

OK.


+
+    total_cpus = num_online_cpus();

Which could change across these invocations.. Perhaps
during these calls we need to lock up CPU up/down code?

OK.


+
+    if ( id == 0 )
+    {

Can you just make this its own function? Perhaps call it
'xsplice_do_single' or such?

+        s_time_t timeout, start;
+
+        /* Trigger other CPUs to execute do_xsplice */

Missing full stop.
+        smp_call_function(reschedule_fn, NULL, 0);
+
+        /* Wait for other CPUs with a timeout */

Missing full stop.
+        start = NOW();
+        timeout = start + MILLISECS(30);

Nah. That should be gotten from the XSPLICE_ACTION_APPLY 'time'
parameter - which has an 'timeout' in it.

+        while ( atomic_read(&xsplice_work.semaphore) != total_cpus &&
+                NOW() < timeout )
+            cpu_relax();
+
+        if ( atomic_read(&xsplice_work.semaphore) == total_cpus )
+        {
+            struct payload *data2;

s/data2/data/ ?
+
+            /* "Mask" NMIs */
+            saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
+
+            /* All CPUs are waiting, now signal to disable IRQs */
+            xsplice_work.ready = true;
+            smp_mb();
+
+            /* Wait for irqs to be disabled */
+            while ( atomic_read(&xsplice_work.irq_semaphore) != (total_cpus - 
1) )
+                cpu_relax();
+
+            local_irq_disable();
+            /* Now this function should be the only one on any stack.
+             * No need to lock the payload list or applied list. */
+            switch ( xsplice_work.cmd )
+            {
+                case XSPLICE_ACTION_APPLY:
+                        xsplice_work.data->rc = 
apply_payload(xsplice_work.data);
+                        if ( xsplice_work.data->rc == 0 )
+                            xsplice_work.data->state = XSPLICE_STATE_APPLIED;
+                        break;
+                case XSPLICE_ACTION_REVERT:
+                        xsplice_work.data->rc = 
revert_payload(xsplice_work.data);
+                        if ( xsplice_work.data->rc == 0 )
+                            xsplice_work.data->state = XSPLICE_STATE_CHECKED;
+                        break;
+                case XSPLICE_ACTION_REPLACE:
+                        list_for_each_entry ( data2, &payload_list, list )
+                        {
+                            if ( data2->state != XSPLICE_STATE_APPLIED )
+                                continue;
+
+                            data2->rc = revert_payload(data2);
+                            if ( data2->rc == 0 )
+                                data2->state = XSPLICE_STATE_CHECKED;
+                            else
+                            {
+                                xsplice_work.data->rc = -EINVAL;

Why not copy the error code (from data2->rc?)

No. Reverting a different payload updates the error code for that payload. The payload to-be-applied has failed because a dependent action has failed. This is not the same as the original error. The original error is visible through data2->rc.

+                                break;
+                            }
+                        }
+                        if ( xsplice_work.data->rc != -EINVAL )

And here you can just check for zero.

No, because xsplice_work.data->rc is originally -EAGAIN (in progress). I suppose I could check for xsplice_work.data->rc == -EAGAIN.

+                        {
+                            xsplice_work.data->rc = 
apply_payload(xsplice_work.data);
+                            if ( xsplice_work.data->rc == 0 )
+                                xsplice_work.data->state = 
XSPLICE_STATE_APPLIED;
+                        }
+                        break;
+                default:
+                        xsplice_work.data->rc = -EINVAL;
+                        break;
+            }
+
+            local_irq_enable();
+            set_nmi_callback(saved_nmi_callback);
+        }
+        else
+        {
+            xsplice_work.data->rc = -EBUSY;
+        }
+
+        xsplice_work.do_work = 0;
+        smp_mb(); /* Synchronize with waiting CPUs */

Missing full stop.
+    }
+    else
+    {
+        /* Wait for all CPUs to rendezvous */

Missing full stop
+        while ( xsplice_work.do_work && !xsplice_work.ready )
+        {
+            cpu_relax();
+            smp_mb();
+        }
+
+        /* Disable IRQs and signal */

Missing full stop.
+        local_irq_disable();
+        atomic_inc(&xsplice_work.irq_semaphore);
+
+        /* Wait for patching to complete */

Missing full stop.
+        while ( xsplice_work.do_work )
+        {
+            cpu_relax();
+            smp_mb();
+        }
+        local_irq_enable();
+    }
+}
+
  static int __init xsplice_init(void)
  {
      register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
diff --git a/xen/include/asm-arm/nmi.h b/xen/include/asm-arm/nmi.h
index a60587e..82aff35 100644
--- a/xen/include/asm-arm/nmi.h
+++ b/xen/include/asm-arm/nmi.h
@@ -4,6 +4,19 @@
  #define register_guest_nmi_callback(a)  (-ENOSYS)
  #define unregister_guest_nmi_callback() (-ENOSYS)

+typedef int (*nmi_callback_t)(const struct cpu_user_regs *regs, int cpu);
+
+/**
+ * set_nmi_callback
+ *
+ * Set a handler for an NMI. Only one handler may be
+ * set. Return the old nmi callback handler.
+ */
+static inline nmi_callback_t set_nmi_callback(nmi_callback_t callback)
+{
+    return NULL;
+}
+
  #endif /* ASM_NMI_H */
  /*
   * Local variables:
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index a3946a3..507829c 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -11,7 +11,8 @@ struct xsplice_patch_func {
      unsigned long old_addr;
      unsigned long old_size;
      char *name;
-    unsigned char undo[8];
+    uint8_t undo[8];
+    uint8_t pad[56];

This should be in a different patch. As part of the
"xsplice: Implement payload loading"

Oops, that's what I intended.

--
Ross Lagerwall

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.