diff mbox series

[v6,07/12] livepatch: Add per-function applied/reverted state tracking marker

Message ID 20191126100801.124844-8-wipawel@amazon.de (mailing list archive)
State New, archived
Headers show
Series livepatch: new features and fixes | expand

Commit Message

Wieczorkiewicz, Pawel Nov. 26, 2019, 10:07 a.m. UTC
Livepatch only tracks an entire payload applied/reverted state. But,
with an option to supply the apply_payload() and/or revert_payload()
functions as optional hooks, it becomes possible to intermix the
execution of the original apply_payload()/revert_payload() functions
with their dynamically supplied counterparts.
It is important then to track the current state of every function
being patched and prevent situations of unintentional double-apply
or unapplied revert.

To support that, it is necessary to extend public interface of the
livepatch. The struct livepatch_func gets additional field holding
the applied/reverted state marker.

To reflect the livepatch payload ABI change, bump the version flag
LIVEPATCH_PAYLOAD_VERSION up to 2.

[And also update the top of the design document]

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v3:
  * Use common_livepatch_{apply,revert} instead of modifying
    arch_livepatch_{apply,revert}.
  * s/bool_t/bool/g

Changed since v2:
  * Documentation fixes

Changed since v1:
  * support the feature for all arch (add handling for Arm)
  * add common is_func_applied() and is_func_reverted() to be
    used by all arch
  * remove explicit enum values from enum livepatch_func_state
  * added corresponding documentation
  * added tests
---
 docs/misc/livepatch.pandoc                     |  17 ++-
 xen/common/livepatch.c                         |  39 ++++++-
 xen/include/public/sysctl.h                    |   9 +-
 xen/include/xen/livepatch.h                    |  28 +++++
 xen/test/livepatch/Makefile                    |  27 ++++-
 xen/test/livepatch/xen_action_hooks.c          |   2 +
 xen/test/livepatch/xen_action_hooks_marker.c   | 112 +++++++++++++++++++
 xen/test/livepatch/xen_action_hooks_noapply.c  | 136 +++++++++++++++++++++++
 xen/test/livepatch/xen_action_hooks_norevert.c | 143 +++++++++++++++++++++++++
 9 files changed, 507 insertions(+), 6 deletions(-)
 create mode 100644 xen/test/livepatch/xen_action_hooks_marker.c
 create mode 100644 xen/test/livepatch/xen_action_hooks_noapply.c
 create mode 100644 xen/test/livepatch/xen_action_hooks_norevert.c

Comments

Andrew Cooper Jan. 5, 2020, 11:36 a.m. UTC | #1
On 26/11/2019 10:07, Pawel Wieczorkiewicz wrote:
> @@ -1274,6 +1297,9 @@ static void livepatch_do_action(void)
>          else
>              rc = apply_payload(data);
>  
> +        if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
> +            panic("livepatch: partially applied payload '%s'!\n", data->name);
> +
>          if ( rc == 0 )
>              apply_payload_tail(data);
>          break;
> @@ -1288,6 +1314,9 @@ static void livepatch_do_action(void)
>          else
>              rc = revert_payload(data);
>  
> +        if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
> +            panic("livepatch: partially reverted payload '%s'!\n", data->name);
> +
>          if ( rc == 0 )
>              revert_payload_tail(data);
>          break;
> @@ -1309,6 +1338,9 @@ static void livepatch_do_action(void)
>              else
>                  other->rc = revert_payload(other);
>  
> +            if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
> +                panic("livepatch: partially reverted payload '%s'!\n", other->name);
> +
>              if ( other->rc == 0 )
>                  revert_payload_tail(other);

Coverity highlights that this contains dead code.

The LIVEPATCH_ACTION_REPLACE case, unlike all others, uses other->rc,
which means the rc ? : check will always pass LIVEPATCH_FUNC_APPLIED
into was_action_consistent(), due to the rc = 0 at the head of the case
block.

If this were the only problem, switching rc to other->rc might be ok,
but there look to be other confusions in the surrounding code.  Would
you mind looking over the whole block of code for correct error handling?

For any resulting patch, the Coverity ID is 1457467

~Andrew

>              else
> @@ -1329,6 +1361,9 @@ static void livepatch_do_action(void)
>              else
>                  rc = apply_payload(data);
>  
> +            if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
> +                panic("livepatch: partially applied payload '%s'!\n", data->name);
> +
>              if ( rc == 0 )
>                  apply_payload_tail(data);
>          }
>
Wieczorkiewicz, Pawel Jan. 6, 2020, 12:22 p.m. UTC | #2
On 5. Jan 2020, at 12:36, Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>> wrote:

On 26/11/2019 10:07, Pawel Wieczorkiewicz wrote:
@@ -1274,6 +1297,9 @@ static void livepatch_do_action(void)
        else
<snip>
        break;
@@ -1309,6 +1338,9 @@ static void livepatch_do_action(void)
            else
                other->rc = revert_payload(other);

+            if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
+                panic("livepatch: partially reverted payload '%s'!\n", other->name);
+
            if ( other->rc == 0 )
                revert_payload_tail(other);

Coverity highlights that this contains dead code.

The LIVEPATCH_ACTION_REPLACE case, unlike all others, uses other->rc,
which means the rc ? : check will always pass LIVEPATCH_FUNC_APPLIED
into was_action_consistent(), due to the rc = 0 at the head of the case
block.


Yes, this has to be other->rc instead of rc. Thanks!

If this were the only problem, switching rc to other->rc might be ok,
but there look to be other confusions in the surrounding code.  Would
you mind looking over the whole block of code for correct error handling?


What are the confusions in the code? Could you be more specific and point me to them?

I have just checked the LIVEPATCH_ACTION_REPLACE case block again.
It looks correct to me. That is, it preserves the original logic of error handling there.
I just added the extensions. But, the flow for rc and other->rc should be the same
and correct (modulo the was_action_consistent() bug).

For any resulting patch, the Coverity ID is 1457467

~Andrew

            else
@@ -1329,6 +1361,9 @@ static void livepatch_do_action(void)
            else
                rc = apply_payload(data);

+            if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
+                panic("livepatch: partially applied payload '%s'!\n", data->name);
+
            if ( rc == 0 )
                apply_payload_tail(data);
        }



Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Andrew Cooper Jan. 6, 2020, 12:25 p.m. UTC | #3
On 06/01/2020 12:22, Wieczorkiewicz, Pawel wrote:
>
>
>> On 5. Jan 2020, at 12:36, Andrew Cooper <andrew.cooper3@citrix.com
>> <mailto:andrew.cooper3@citrix.com>> wrote:
>>
>> On 26/11/2019 10:07, Pawel Wieczorkiewicz wrote:
>>> @@ -1274,6 +1297,9 @@ static void livepatch_do_action(void)
>>>         else
> <snip>
>>>         break;
>>> @@ -1309,6 +1338,9 @@ static void livepatch_do_action(void)
>>>             else
>>>                 other->rc = revert_payload(other);
>>>
>>> +            if ( !was_action_consistent(other, rc ?
>>> LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
>>> +                panic("livepatch: partially reverted payload
>>> '%s'!\n", other->name);
>>> +
>>>             if ( other->rc == 0 )
>>>                 revert_payload_tail(other);
>>
>> Coverity highlights that this contains dead code.
>>
>> The LIVEPATCH_ACTION_REPLACE case, unlike all others, uses other->rc,
>> which means the rc ? : check will always pass LIVEPATCH_FUNC_APPLIED
>> into was_action_consistent(), due to the rc = 0 at the head of the case
>> block.
>>
>
> Yes, this has to be other->rc instead of rc. Thanks!
>
>> If this were the only problem, switching rc to other->rc might be ok,
>> but there look to be other confusions in the surrounding code.  Would
>> you mind looking over the whole block of code for correct error handling?
>>
>
> What are the confusions in the code? Could you be more specific and
> point me to them?
>
> I have just checked the LIVEPATCH_ACTION_REPLACE case block again.
> It looks correct to me. That is, it preserves the original logic of
> error handling there.
> I just added the extensions. But, the flow for rc and other->rc should
> be the same
> and correct (modulo the was_action_consistent() bug).

So long as you've double checked and you think the rest of the logic is
fine, then ok.  It was the further mixed use of rc vs other->rc which I
was concerned about.

~Andrew
diff mbox series

Patch

diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index 3c635add92..02348dd49e 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -1,4 +1,4 @@ 
-# Xen Live Patching Design v1
+# Xen Live Patching Design v2
 
 ## Rationale
 
@@ -297,10 +297,14 @@  which describe the functions to be patched:
         uint32_t old_size;
         uint8_t version;
         uint8_t opaque[31];
+        /* Added to livepatch payload version 2: */
+        uint8_t applied;
+        uint8_t _pad[7];
     };
 
 The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 52 on 32-bit hypervisors.
+The version 2 of the payload adds additional 8 bytes to the structure size.
 
  * `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
@@ -324,9 +328,15 @@  The size of the structure is 64 bytes on 64-bit hypervisors. It will be
    * If the value of `new_addr` is zero then `new_size` determines how many
     instruction bytes to NOP (up to opaque size modulo smallest platform
     instruction - 1 byte x86 and 4 bytes on ARM).
- * `version` is to be one.
+ * `version` indicates version of the generated payload.
  * `opaque` **MUST** be zero.
 
+The version 2 of the payload adds the following fields to the structure:
+
+  * `applied` tracks function's applied/reverted state. It has a boolean type
+    either LIVEPATCH_FUNC_NOT_APPLIED or LIVEPATCH_FUNC_APPLIED.
+  * `_pad[7]` adds padding to align to 8 bytes.
+
 The size of the `livepatch_func` array is determined from the ELF section
 size.
 
@@ -378,6 +388,9 @@  A simple example of what a payload file can be:
         uint32_t old_size;
         uint8_t version;
         uint8_t pad[31];
+        /* Added to livepatch payload version 2: */
+        uint8_t applied;
+        uint8_t _pad[7];
     };
 
     /* Our replacement function for xen_extra_version. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index add7da7fa9..81e1b474cd 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1182,7 +1182,7 @@  static int apply_payload(struct payload *data)
     ASSERT(!local_irq_is_enabled());
 
     for ( i = 0; i < data->nfuncs; i++ )
-        arch_livepatch_apply(&data->funcs[i]);
+        common_livepatch_apply(&data->funcs[i]);
 
     arch_livepatch_revive();
 
@@ -1216,7 +1216,7 @@  static int revert_payload(struct payload *data)
     }
 
     for ( i = 0; i < data->nfuncs; i++ )
-        arch_livepatch_revert(&data->funcs[i]);
+        common_livepatch_revert(&data->funcs[i]);
 
     /*
      * Since we are running with IRQs disabled and the hooks may call common
@@ -1248,6 +1248,29 @@  static inline void revert_payload_tail(struct payload *data)
     data->state = LIVEPATCH_STATE_CHECKED;
 }
 
+/*
+ * Check if an action has applied the same state to all payload's functions consistently.
+ */
+static inline bool was_action_consistent(const struct payload *data, livepatch_func_state_t expected_state)
+{
+    int i;
+
+    for ( i = 0; i < data->nfuncs; i++ )
+    {
+        struct livepatch_func *f = &(data->funcs[i]);
+
+        if ( f->applied != expected_state )
+        {
+            printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n",
+                   data->name, f->name ?: "noname");
+
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /*
  * This function is executed having all other CPUs with no deep stack (we may
  * have cpu_idle on it) and IRQs disabled.
@@ -1274,6 +1297,9 @@  static void livepatch_do_action(void)
         else
             rc = apply_payload(data);
 
+        if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
+            panic("livepatch: partially applied payload '%s'!\n", data->name);
+
         if ( rc == 0 )
             apply_payload_tail(data);
         break;
@@ -1288,6 +1314,9 @@  static void livepatch_do_action(void)
         else
             rc = revert_payload(data);
 
+        if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
+            panic("livepatch: partially reverted payload '%s'!\n", data->name);
+
         if ( rc == 0 )
             revert_payload_tail(data);
         break;
@@ -1309,6 +1338,9 @@  static void livepatch_do_action(void)
             else
                 other->rc = revert_payload(other);
 
+            if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
+                panic("livepatch: partially reverted payload '%s'!\n", other->name);
+
             if ( other->rc == 0 )
                 revert_payload_tail(other);
             else
@@ -1329,6 +1361,9 @@  static void livepatch_do_action(void)
             else
                 rc = apply_payload(data);
 
+            if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
+                panic("livepatch: partially applied payload '%s'!\n", data->name);
+
             if ( rc == 0 )
                 apply_payload_tail(data);
         }
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 7a0884b70b..4f52ad581f 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -832,7 +832,7 @@  struct xen_sysctl_cpu_featureset {
  *     If zero exit with success.
  */
 
-#define LIVEPATCH_PAYLOAD_VERSION 1
+#define LIVEPATCH_PAYLOAD_VERSION 2
 /*
  * .livepatch.funcs structure layout defined in the `Payload format`
  * section in the Live Patch design document.
@@ -840,6 +840,11 @@  struct xen_sysctl_cpu_featureset {
  * We guard this with __XEN__ as toolstacks SHOULD not use it.
  */
 #ifdef __XEN__
+typedef enum livepatch_func_state {
+    LIVEPATCH_FUNC_NOT_APPLIED,
+    LIVEPATCH_FUNC_APPLIED
+} livepatch_func_state_t;
+
 struct livepatch_func {
     const char *name;       /* Name of function to be patched. */
     void *new_addr;
@@ -848,6 +853,8 @@  struct livepatch_func {
     uint32_t old_size;
     uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
     uint8_t opaque[31];
+    uint8_t applied;
+    uint8_t _pad[7];
 };
 typedef struct livepatch_func livepatch_func_t;
 #endif
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 3a91626a79..f3ae10f007 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -123,6 +123,34 @@  void arch_livepatch_post_action(void);
 
 void arch_livepatch_mask(void);
 void arch_livepatch_unmask(void);
+
+static inline void common_livepatch_apply(struct livepatch_func *func)
+{
+    /* If the action has been already executed on this function, do nothing. */
+    if ( func->applied == LIVEPATCH_FUNC_APPLIED )
+    {
+        printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n",
+                __func__, func->name);
+        return;
+    }
+
+    arch_livepatch_apply(func);
+    func->applied = LIVEPATCH_FUNC_APPLIED;
+}
+
+static inline void common_livepatch_revert(struct livepatch_func *func)
+{
+    /* If the apply action hasn't been executed on this function, do nothing. */
+    if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED )
+    {
+        printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n",
+                __func__, func->name);
+        return;
+    }
+
+    arch_livepatch_revert(func);
+    func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
+}
 #else
 
 /*
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index bbc6bdaf64..23113d3418 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -24,6 +24,9 @@  LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
 LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
 LIVEPATCH_ACTION_HOOKS := xen_action_hooks.livepatch
 LIVEPATCH_ACTION_HOOKS_NOFUNC := xen_action_hooks_nofunc.livepatch
+LIVEPATCH_ACTION_HOOKS_MARKER:= xen_action_hooks_marker.livepatch
+LIVEPATCH_ACTION_HOOKS_NOAPPLY:= xen_action_hooks_noapply.livepatch
+LIVEPATCH_ACTION_HOOKS_NOREVERT:= xen_action_hooks_norevert.livepatch
 
 LIVEPATCHES += $(LIVEPATCH)
 LIVEPATCHES += $(LIVEPATCH_BYE)
@@ -34,6 +37,9 @@  LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
 LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
 LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS)
 LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_MARKER)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
 
 LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
 
@@ -158,7 +164,26 @@  $(LIVEPATCH_ACTION_HOOKS): xen_action_hooks.o xen_hello_world_func.o note.o xen_
 $(LIVEPATCH_ACTION_HOOKS_NOFUNC): xen_action_hooks_nofunc.o note.o xen_note.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $^
 
+xen_actions_hooks_marker.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_MARKER)
+$(LIVEPATCH_ACTION_HOOKS_MARKER): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_MARKER) $^
+
+xen_actions_hooks_noapply.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
+$(LIVEPATCH_ACTION_HOOKS_NOAPPLY): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) $^
+
+xen_actions_hooks_norevert.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
+$(LIVEPATCH_ACTION_HOOKS_NOREVERT): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $^
+
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
            $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS) \
-           $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
+           $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $(LIVEPATCH_ACTION_HOOKS_MARKER) $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) \
+           $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
diff --git a/xen/test/livepatch/xen_action_hooks.c b/xen/test/livepatch/xen_action_hooks.c
index a947afc41f..39b5313027 100644
--- a/xen/test/livepatch/xen_action_hooks.c
+++ b/xen/test/livepatch/xen_action_hooks.c
@@ -28,6 +28,7 @@  static int apply_hook(livepatch_payload_t *payload)
     {
         struct livepatch_func *func = &payload->funcs[i];
 
+        func->applied = LIVEPATCH_FUNC_APPLIED;
         apply_cnt++;
 
         printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
@@ -48,6 +49,7 @@  static int revert_hook(livepatch_payload_t *payload)
     {
         struct livepatch_func *func = &payload->funcs[i];
 
+        func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
         revert_cnt++;
 
         printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
diff --git a/xen/test/livepatch/xen_action_hooks_marker.c b/xen/test/livepatch/xen_action_hooks_marker.c
new file mode 100644
index 0000000000..4f807a577f
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_marker.c
@@ -0,0 +1,112 @@ 
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#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 const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+    .version = LIVEPATCH_PAYLOAD_VERSION,
+    .name = hello_world_patch_this_fnc,
+    .new_addr = xen_hello_world,
+    .old_addr = xen_extra_version,
+    .new_size = NEW_CODE_SZ,
+    .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_action_hooks_noapply.c b/xen/test/livepatch/xen_action_hooks_noapply.c
new file mode 100644
index 0000000000..4c55c156a6
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_noapply.c
@@ -0,0 +1,136 @@ 
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#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 const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int apply_cnt;
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static int apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        apply_cnt++;
+        printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return -EINVAL; /* Mark action as inconsistent */
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(apply_cnt != 1);
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(apply_hook);
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+    .version = LIVEPATCH_PAYLOAD_VERSION,
+    .name = hello_world_patch_this_fnc,
+    .new_addr = xen_hello_world,
+    .old_addr = xen_extra_version,
+    .new_size = NEW_CODE_SZ,
+    .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c
new file mode 100644
index 0000000000..ef77e72071
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_norevert.c
@@ -0,0 +1,143 @@ 
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#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 const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int revert_cnt;
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+        printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return 0;
+}
+
+static int revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        revert_cnt++;
+        printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return -EINVAL; /* Mark action as inconsistent */
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+    int i;
+
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+    for (i = 0; i < payload->nfuncs; i++)
+    {
+        struct livepatch_func *func = &payload->funcs[i];
+
+        BUG_ON(revert_cnt != 1);
+        BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+
+        /* Outside of quiesce zone: MAY TRIGGER HOST CRASH/UNDEFINED BEHAVIOR */
+        arch_livepatch_quiesce();
+        common_livepatch_revert(payload);
+        arch_livepatch_revive();
+        BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+
+        printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(revert_hook);
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+    .version = LIVEPATCH_PAYLOAD_VERSION,
+    .name = hello_world_patch_this_fnc,
+    .new_addr = xen_hello_world,
+    .old_addr = xen_extra_version,
+    .new_size = NEW_CODE_SZ,
+    .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */