|
[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 Thu, Nov 05, 2015 at 11:45:42AM +0000, Ross Lagerwall wrote:
> 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?
I was thinking of page being read-only and we hadn't modified it (bug
in our code). It would be good to actually report this instead of
doing an #GP and crashing the whole hypervisor.
> >>+ *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.
OK. My thoughts were that if this gets wedged - the user may want
to hit 'x' on the serial console to get an idea of what is wedged.
And the keyhandler can print the gory details of the payloads - along
with the one that is currently wedged and what command it is currently
doing. Having this in payload->cmd would work nicely.
But on the other hand - we could also just expand the keyhandler to
look in the xsplice_work and print the gory details of that.
> 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.
Right, but previously the 'cmd' had the command it was to perform.
>
> >
> >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);
You also need to verify that the 'sh_size' is actually the right aligned
size. As in we don't want to have a size that when dividing by 'sizeof
*payload->funcs' gives us a remainder.
> >>+
> >>+ 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.
The title of the patch sounded like it would add the infrastructure
for patching. Not the pieces needed from ELF file.
But thay may be just me.
>
> >>+
> >> 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.
Sorry, it is the 'ftrace_modify_code_direct' - see 'sync_core' which
does an cpuid.
>
> >
> >>+ 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.
/me nods. That would be nicer I think?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |