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

Re: [Xen-devel] [PATCH v3 07/23] xsplice: Implement support for applying/reverting/replacing patches. (v5)

On 02/16/2016 07:11 PM, Andrew Cooper wrote:
On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote:
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..b5995b9 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -36,6 +36,7 @@
  #include <xen/cpu.h>
  #include <xen/wait.h>
  #include <xen/guest_access.h>
+#include <xen/xsplice.h>
  #include <public/sysctl.h>
  #include <public/hvm/hvm_vcpu.h>
  #include <asm/regs.h>
@@ -121,6 +122,7 @@ static void idle_loop(void)
+        do_xsplice(); /* Must be last. */

Then name "do_xsplice()" is slightly misleading (although it is in equal
company here).  check_for_xsplice_work() would be more accurate.

diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
index 814dd52..ae35e91 100644
--- a/xen/arch/x86/xsplice.c
+++ b/xen/arch/x86/xsplice.c
@@ -10,6 +10,25 @@
                              __func__,__LINE__, x); return x; }

+#define PATCH_INSN_SIZE 5

Somewhere you should have a BUILD_BUG_ON() confirming that
PATCH_INSN_SIZE fits within the undo array.

Having said that, I think all of xsplice_patch_func should be
arch-specific rather than generic.

+void xsplice_apply_jmp(struct xsplice_patch_func *func)
+    uint32_t val;
+    uint8_t *old_ptr;
+    old_ptr = (uint8_t *)func->old_addr;
+    memcpy(func->undo, old_ptr, PATCH_INSN_SIZE);

At least a newline here please.

+    *old_ptr++ = 0xe9; /* Relative jump */
+    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;

E9 takes a rel32 parameter, which is signed.

I think you need to explicitly cast to intptr_t and used signed
arithmetic throughout this calculation to correctly calculate a
backwards jump.

According to my testing and expectations based on the spec and GCC's implementation-defined behaviour, the offset is computed correctly for backward (and forward) jumps. I'm sure the types can be improved though...

I think there should also be some sanity checks that both old_addr and
new_addr are in the Xen 1G virtual region.

OK. Though these sanity checks should happen when loading the patch, not applying it.

Ross Lagerwall

Xen-devel mailing list



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