diff mbox series

[v3,04/12] livepatch: Implement pre-|post- apply|revert hooks

Message ID 20190916105945.93632-5-wipawel@amazon.de (mailing list archive)
State Superseded
Headers show
Series livepatch: new features and fixes | expand

Commit Message

Wieczorkiewicz, Pawel Sept. 16, 2019, 10:59 a.m. UTC
This is an implementation of 4 new livepatch module vetoing hooks,
that can be optionally supplied along with modules.
Hooks that currently exists in the livepatch mechanism aren't agile
enough and have various limitations:
* run only from within a quiescing zone
* cannot conditionally prevent applying or reverting
* do not have access to the module context
To address these limitations the following has been implemented:
1) pre-apply hook
  runs before the apply action is scheduled for execution. Its main
  purpose is to prevent from applying a hotpatch when certain
  expected conditions aren't met or when mutating actions implemented
  in the hook fail or cannot be executed.

2) post-apply hook
  runs after the apply action has been executed and quiescing zone
  exited. Its main purpose is to provide an ability to follow-up on
  actions performed by the pre- hook, when module application was
  successful or undo certain preparation steps of the pre- hook in
  case of a failure. The success/failure error code is proviVded to
  the post- hooks via the rc field of the payload structure.

3) pre-revert hook
  runs before the revert action is scheduled for execution. Its main
  purpose is to prevent from reverting a hotpatch when certain
  expected conditions aren't met or when mutating actions implemented
  in the hook fail or cannot be executed.

4) post-revert hook
  runs after the revert action has been executed and quiescing zone
  exited. Its main purpose is to perform cleanup of all previously
  executed mutating actions in order to restore the original system
  state from before the current module application.
  The success/failure error code is provided to the post- hooks via
  the rc field of the payload structure.

The replace action performs atomically the following actions:
- revert all applied modules
- apply a single replacement module.
With the vetoing hooks in place various inter-hook dependencies may
arise. Also, during the revert part of the operation certain vetoing
hooks may detect failing conditions that previously were satisfied.
That could in turn lead to situation when the revert part must be
rolled back with all the pre- and post- hooks re-applied, which again
can't be guaranteed to always succeed.
The simplest response to this complication is to disallow the replace
action completely on modules with vetoing hooks.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Petre Eftime <epetre@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Changed since v1:
  * added corresponding documentation
  * added tests

 .gitignore                                  |   6 +-
 docs/misc/livepatch.pandoc                  |  90 +++++++++++++-
 xen/common/livepatch.c                      | 179 +++++++++++++++++++++++++---
 xen/include/xen/livepatch_payload.h         |  27 +++++
 xen/test/livepatch/Makefile                 |  19 ++-
 xen/test/livepatch/xen_prepost_hooks.c      | 122 +++++++++++++++++++
 xen/test/livepatch/xen_prepost_hooks_fail.c |  75 ++++++++++++
 7 files changed, 492 insertions(+), 26 deletions(-)
 create mode 100644 xen/test/livepatch/xen_prepost_hooks.c
 create mode 100644 xen/test/livepatch/xen_prepost_hooks_fail.c

Comments

Ross Lagerwall Sept. 16, 2019, 5:54 p.m. UTC | #1
On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
> This is an implementation of 4 new livepatch module vetoing hooks,
> that can be optionally supplied along with modules.
> Hooks that currently exists in the livepatch mechanism aren't agile
> enough and have various limitations:
> * run only from within a quiescing zone
> * cannot conditionally prevent applying or reverting
> * do not have access to the module context
> To address these limitations the following has been implemented:
> 1) pre-apply hook
>    runs before the apply action is scheduled for execution. Its main
>    purpose is to prevent from applying a hotpatch when certain
>    expected conditions aren't met or when mutating actions implemented
>    in the hook fail or cannot be executed.
> 
> 2) post-apply hook
>    runs after the apply action has been executed and quiescing zone
>    exited. Its main purpose is to provide an ability to follow-up on
>    actions performed by the pre- hook, when module application was
>    successful or undo certain preparation steps of the pre- hook in
>    case of a failure. The success/failure error code is proviVded to

provided

>    the post- hooks via the rc field of the payload structure.
> 
> 3) pre-revert hook
>    runs before the revert action is scheduled for execution. Its main
>    purpose is to prevent from reverting a hotpatch when certain

Let's stick with "livepatch" terminology to avoid confusion (throughout 
this patch).

>    expected conditions aren't met or when mutating actions implemented
>    in the hook fail or cannot be executed.
> 
> 4) post-revert hook
>    runs after the revert action has been executed and quiescing zone
>    exited. Its main purpose is to perform cleanup of all previously
>    executed mutating actions in order to restore the original system
>    state from before the current module application.
>    The success/failure error code is provided to the post- hooks via
>    the rc field of the payload structure.
> 

snip

> +/*
> + * Check if payload has any of the vetoing, non-atomic hooks assigned.
> + * A vetoing, non-atmic hook may perform an operation that changes the
> + * hypervisor state and may not be guaranteed to succeed. Result of
> + * such operation may be returned and may change the livepatch workflow.
> + * Such hooks may require additional cleanup actions performed by other
> + * hooks. Thus they are not suitable for replace action.
> + */
> +static inline bool_t has_payload_any_vetoing_hooks(const struct payload *payload)

Use bool instead (throughout this patch).

> +{
> +    return is_hook_enabled(payload->hooks.apply.pre) ||
> +           is_hook_enabled(payload->hooks.apply.post) ||
> +           is_hook_enabled(payload->hooks.revert.pre) ||
> +           is_hook_enabled(payload->hooks.revert.post);
> +}
> +
> +/*
> + * Checks if any of the already applied hotpatches has any vetoing,
> + * non-atomic hooks assigned.
> + */
snip

> @@ -1560,6 +1681,30 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
>               rc = build_id_dep(data, 1 /* against hypervisor. */);
>               if ( rc )
>                   break;
> +
> +            /*
> +             * REPLACE action is not supported on hotpatches with vetoing hooks.
> +             * Vetoing hooks usually perform mutating actions on the system and
> +             * typically exist in pairs (pre- hook doing an action and post- hook
> +             * undoing the action). Coalescing all hooks from all applied modules
> +             * cannot be performed without inspecting potential dependencies between
> +             * the mutating hooks and hence cannot be performed automatically by
> +             * the replace action. Also, the replace action cannot safely assume a
> +             * successful revert of all the module with vetoing hooks. When one
> +             * of the hooks fails due to not meeting certain conditions the whole
> +             * replace operation must have been reverted with all previous pre- and
> +             * post- hooks re-executed (which cannot be guaranteed to succeed).
> +             * The simplest response to this complication is disallow replace
> +             * action on modules with vetoing hooks.
> +             */

I think that allowing pre-apply veto hooks would be useful for the 
replace action so the live patch can check if the system is in a good 
state before doing the replace (this would certainly be useful for 
XenServer). It would be safe as far as I can see with the caveat that it 
can't mutate state. But this doesn't have to be done now.

> +            if ( has_payload_any_vetoing_hooks(data) || livepatch_applied_have_vetoing_hooks() )
> +            {
> +                printk(XENLOG_ERR LIVEPATCH "%s: REPLACE action is not supported on hotpatches with vetoing hooks!\n",
> +                       data->name);
> +                rc = -EOPNOTSUPP;
> +                break;
> +            }
> +
>               data->rc = -EAGAIN;
>               rc = schedule_work(data, action->cmd, action->timeout);
>           }
> diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
> index 99613af2db..cd20944cc4 100644
> --- a/xen/include/xen/livepatch_payload.h
> +++ b/xen/include/xen/livepatch_payload.h
> @@ -21,6 +21,16 @@ typedef struct payload livepatch_payload_t;
>   typedef void livepatch_loadcall_t(void);
>   typedef void livepatch_unloadcall_t(void);
>   
> +typedef int livepatch_precall_t(livepatch_payload_t *arg);
> +typedef void livepatch_postcall_t(livepatch_payload_t *arg);
> +
> +struct livepatch_hooks {
> +    struct {
> +        livepatch_precall_t *const *pre;
> +        livepatch_postcall_t *const *post;

Wouldn't it be simpler to drop a level of indirection here?

> +    } apply, revert;
> +};
> +
>   struct payload {
>       uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
>       int32_t rc;                          /* 0 or -XEN_EXX. */
> @@ -47,6 +57,7 @@ struct payload {
>       struct livepatch_build_id xen_dep;   /* ELFNOTE_DESC(.livepatch.xen_depends). */
>       livepatch_loadcall_t *const *load_funcs;   /* The array of funcs to call after */
>       livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
> +    struct livepatch_hooks hooks;        /* Pre and post hooks for apply and revert */
>       unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
>       unsigned int n_unload_funcs;         /* Nr of funcs to call durung unload. */
>       char name[XEN_LIVEPATCH_NAME_SIZE];  /* Name of it. */
> @@ -76,6 +87,22 @@ struct payload {
>        livepatch_unloadcall_t *__weak \
>           const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
>   
> +#define LIVEPATCH_PREAPPLY_HOOK(_fn) \
> +    livepatch_precall_t *__attribute__((weak, used)) \
> +        const livepatch_preapply_data_##_fn __section(".livepatch.hooks.preapply") = _fn;
> +
> +#define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
> +    livepatch_postcall_t *__attribute__((weak, used)) \
> +        const livepatch_postapply_data_##_fn __section(".livepatch.hooks.postapply") = _fn;
> +
> +#define LIVEPATCH_PREREVERT_HOOK(_fn) \
> +    livepatch_precall_t *__attribute__((weak, used)) \
> +        const livepatch_prerevert_data_##_fn __section(".livepatch.hooks.prerevert") = _fn;
> +
> +#define LIVEPATCH_POSTREVERT_HOOK(_fn) \
> +    livepatch_postcall_t *__attribute__((weak, used)) \
> +        const livepatch_postrevert_data_##_fn __section(".livepatch.hooks.postrevert") = _fn;
> +
>   #endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
>   
>   /*
Thanks,
Wieczorkiewicz, Pawel Sept. 17, 2019, 9:12 a.m. UTC | #2
> On 16. Sep 2019, at 19:54, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
>> 
snip
>> 2) post-apply hook
>>   runs after the apply action has been executed and quiescing zone
>>   exited. Its main purpose is to provide an ability to follow-up on
>>   actions performed by the pre- hook, when module application was
>>   successful or undo certain preparation steps of the pre- hook in
>>   case of a failure. The success/failure error code is proviVded to
> 
> provided

ACK.

> 
>>   the post- hooks via the rc field of the payload structure.
>> 3) pre-revert hook
>>   runs before the revert action is scheduled for execution. Its main
>>   purpose is to prevent from reverting a hotpatch when certain
> 
> Let's stick with "livepatch" terminology to avoid confusion (throughout this patch).
> 

ACK.

>>   expected conditions aren't met or when mutating actions implemented
>>   in the hook fail or cannot be executed.
>> 4) post-revert hook
>>   runs after the revert action has been executed and quiescing zone
>>   exited. Its main purpose is to perform cleanup of all previously
>>   executed mutating actions in order to restore the original system
>>   state from before the current module application.
>>   The success/failure error code is provided to the post- hooks via
>>   the rc field of the payload structure.
> 
> snip
> 
>> +/*
>> + * Check if payload has any of the vetoing, non-atomic hooks assigned.
>> + * A vetoing, non-atmic hook may perform an operation that changes the
>> + * hypervisor state and may not be guaranteed to succeed. Result of
>> + * such operation may be returned and may change the livepatch workflow.
>> + * Such hooks may require additional cleanup actions performed by other
>> + * hooks. Thus they are not suitable for replace action.
>> + */
>> +static inline bool_t has_payload_any_vetoing_hooks(const struct payload *payload)
> 
> Use bool instead (throughout this patch).

ACK.

> 
>> +{
>> +    return is_hook_enabled(payload->hooks.apply.pre) ||
>> +           is_hook_enabled(payload->hooks.apply.post) ||
>> +           is_hook_enabled(payload->hooks.revert.pre) ||
>> +           is_hook_enabled(payload->hooks.revert.post);
>> +}
>> +
>> +/*
>> + * Checks if any of the already applied hotpatches has any vetoing,
>> + * non-atomic hooks assigned.
>> + */
> snip
> 
>> @@ -1560,6 +1681,30 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
>>              rc = build_id_dep(data, 1 /* against hypervisor. */);
>>              if ( rc )
>>                  break;
>> +
>> +            /*
>> +             * REPLACE action is not supported on hotpatches with vetoing hooks.
>> +             * Vetoing hooks usually perform mutating actions on the system and
>> +             * typically exist in pairs (pre- hook doing an action and post- hook
>> +             * undoing the action). Coalescing all hooks from all applied modules
>> +             * cannot be performed without inspecting potential dependencies between
>> +             * the mutating hooks and hence cannot be performed automatically by
>> +             * the replace action. Also, the replace action cannot safely assume a
>> +             * successful revert of all the module with vetoing hooks. When one
>> +             * of the hooks fails due to not meeting certain conditions the whole
>> +             * replace operation must have been reverted with all previous pre- and
>> +             * post- hooks re-executed (which cannot be guaranteed to succeed).
>> +             * The simplest response to this complication is disallow replace
>> +             * action on modules with vetoing hooks.
>> +             */
> 
> I think that allowing pre-apply veto hooks would be useful for the replace action so the live patch can check if the system is in a good state before doing the replace (this would certainly be useful for XenServer). It would be safe as far as I can see with the caveat that it can't mutate state. But this doesn't have to be done now.

Yes, I agree that allowing pre-apply hook makes sense for replace action and could be pretty useful.
Especially, that it runs before quiescing the CPUs, so any mutating actions must be chosen with care anyway.

Let’s create another patch for this, after the current patchset is merged.

> 
>> +            if ( has_payload_any_vetoing_hooks(data) || livepatch_applied_have_vetoing_hooks() )
>> +            {
>> +                printk(XENLOG_ERR LIVEPATCH "%s: REPLACE action is not supported on hotpatches with vetoing hooks!\n",
>> +                       data->name);
>> +                rc = -EOPNOTSUPP;
>> +                break;
>> +            }
>> +
>>              data->rc = -EAGAIN;
>>              rc = schedule_work(data, action->cmd, action->timeout);
>>          }
>> diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
>> index 99613af2db..cd20944cc4 100644
>> --- a/xen/include/xen/livepatch_payload.h
>> +++ b/xen/include/xen/livepatch_payload.h
>> @@ -21,6 +21,16 @@ typedef struct payload livepatch_payload_t;
>>  typedef void livepatch_loadcall_t(void);
>>  typedef void livepatch_unloadcall_t(void);
>>  +typedef int livepatch_precall_t(livepatch_payload_t *arg);
>> +typedef void livepatch_postcall_t(livepatch_payload_t *arg);
>> +
>> +struct livepatch_hooks {
>> +    struct {
>> +        livepatch_precall_t *const *pre;
>> +        livepatch_postcall_t *const *post;
> 
> Wouldn't it be simpler to drop a level of indirection here?

I think it would complicate things, because the handling of original hooks (load, unload)
has been implemented as a pointer to an array of pointers (because of the multiple hooks requirement).

I did not want to introduce a distinction between pointers to multiple hooks and single hooks for simplicity
(all the hooks are arrays of pointers, even if there is only a single hook).

I believe that makes the whole code a bit more consistent and potentially re-usable.

> 
>> +    } apply, revert;
>> +};
>> +
>> 
snip
>>  #endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
>>    /*
> Thanks,
> -- 
> Ross Lagerwall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Ross Lagerwall Sept. 17, 2019, 9:15 a.m. UTC | #3
On 9/17/19 10:12 AM, Wieczorkiewicz, Pawel wrote:
>>> diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
>>> index 99613af2db..cd20944cc4 100644
>>> --- a/xen/include/xen/livepatch_payload.h
>>> +++ b/xen/include/xen/livepatch_payload.h
>>> @@ -21,6 +21,16 @@ typedef struct payload livepatch_payload_t;
>>>   typedef void livepatch_loadcall_t(void);
>>>   typedef void livepatch_unloadcall_t(void);
>>>   +typedef int livepatch_precall_t(livepatch_payload_t *arg);
>>> +typedef void livepatch_postcall_t(livepatch_payload_t *arg);
>>> +
>>> +struct livepatch_hooks {
>>> +    struct {
>>> +        livepatch_precall_t *const *pre;
>>> +        livepatch_postcall_t *const *post;
>>
>> Wouldn't it be simpler to drop a level of indirection here?
> 
> I think it would complicate things, because the handling of original hooks (load, unload)
> has been implemented as a pointer to an array of pointers (because of the multiple hooks requirement).
> 
> I did not want to introduce a distinction between pointers to multiple hooks and single hooks for simplicity
> (all the hooks are arrays of pointers, even if there is only a single hook).
> 
> I believe that makes the whole code a bit more consistent and potentially re-usable.
> 
OK, fair enough.
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 6f83fc8728..5b80e2bbd5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -308,11 +308,7 @@  xen/include/xen/acm_policy.h
 xen/include/xen/compile.h
 xen/include/xen/lib/x86/cpuid-autogen.h
 xen/test/livepatch/config.h
-xen/test/livepatch/xen_bye_world.livepatch
-xen/test/livepatch/xen_hello_world.livepatch
-xen/test/livepatch/xen_nop.livepatch
-xen/test/livepatch/xen_replace_world.livepatch
-xen/test/livepatch/xen_no_xen_buildid.livepatch
+xen/test/livepatch/*.livepatch
 xen/tools/kconfig/.tmp_gtkcheck
 xen/tools/kconfig/.tmp_qtcheck
 xen/tools/symbols
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index fd1f5d0126..ae8566eb30 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -23,6 +23,9 @@  The document is split in four sections:
  * payload - telemetries of the old code along with binary blob of the new
    function (if needed).
  * reloc - telemetries contained in the payload to construct proper trampoline.
+ * hook - an auxiliary function being called before, during or after payload
+          application or revert.
+ * quiescing zone - period when all CPUs are lock-step with each other.
 
 ## History
 
@@ -270,6 +273,10 @@  like what the Linux kernel module loader does.
 The payload contains at least three sections:
 
  * `.livepatch.funcs` - which is an array of livepatch_func structures.
+   and/or any of:
+ * `.livepatch.hooks.{preapply,postapply,prerevert,postrevert}'
+   - which are a pointer to a hook function pointer.
+
  * `.livepatch.xen_depends` - which is an ELF Note that describes what Xen
     build-id the payload depends on. **MUST** have one.
  * `.livepatch.depends` - which is an ELF Note that describes what the payload
@@ -330,12 +337,24 @@  When reverting a patch, the hypervisor iterates over each `livepatch_func`
 and the core code copies the data from the undo buffer (private internal copy)
 to `old_addr`.
 
-It optionally may contain the address of functions to be called right before
-being applied and after being reverted:
+It optionally may contain the address of hooks to be called right before
+being applied and after being reverted (while all CPUs are still in quiescing
+zone). These hooks do not have access to payload structure.
 
  * `.livepatch.hooks.load` - an array of function pointers.
  * `.livepatch.hooks.unload` - an array of function pointers.
 
+It optionally may also contain the address of pre- and post- vetoing hooks to
+be called before (pre) or after (post) apply and revert payload actions (while
+all CPUs are already released from quiescing zone). These hooks do have
+access to payload structure. The pre-apply hook can prevent from loading the
+payload if encoded in it condition is not met. Accordingly, the pre-revert
+hook can prevent from unloading the hotpatch if encoded in it condition is not
+met.
+
+ * `.livepatch.hooks.{preapply,postapply}`
+ * `.livepatch.hooks.{prerevert,postrevert}`
+   - which are a pointer to a single hook function pointer.
 
 ### Example of .livepatch.funcs
 
@@ -371,7 +390,9 @@  A simple example of what a payload file can be:
 
 Code must be compiled with `-fPIC`.
 
-### .livepatch.hooks.load and .livepatch.hooks.unload
+### Hooks
+
+#### .livepatch.hooks.load and .livepatch.hooks.unload
 
 This section contains an array of function pointers to be executed
 before payload is being applied (.livepatch.funcs) or after reverting
@@ -385,6 +406,69 @@  The type definition of the function are as follow:
     typedef void (*livepatch_loadcall_t)(void);
     typedef void (*livepatch_unloadcall_t)(void);
 
+#### .livepatch.hooks.preapply
+
+This section contains a pointer to a single function pointer to be executed
+before apply action is scheduled (and thereby before CPUs are put into
+quiescing zone). This is useful to prevent from applying a payload when
+certain expected conditions aren't met or when mutating actions implemented
+in the hook fail or cannot be executed.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+    typedef int livepatch_precall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.postapply
+
+This section contains a pointer to a single function pointer to be executed
+after apply action has finished and after all CPUs left the quiescing zone.
+This is useful to provide an ability to follow up on actions performed by
+the preapply hook. Especially, when module application was successful or to
+be able to undo certain preparation steps of the preapply hook in case of a
+failure. The success/failure error code is provided to the postapply hooks
+via the `rc` field of the payload structure.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+    typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.prerevert
+
+This section contains a pointer to a single function pointer to be executed
+before revert action is scheduled (and thereby before CPUs are put into
+quiescing zone). This is useful to prevent from reverting a payload when
+certain expected conditions aren't met or when mutating actions implemented
+in the hook fail or cannot be executed.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+    typedef int livepatch_precall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.postrevert
+
+This section contains a pointer to a single function pointer to be executed
+after revert action has finished and after all CPUs left the quiescing zone.
+This is useful to provide an ability to perform cleanup of all previously
+executed mutating actions in order to restore the original system state from
+before the current payload application. The success/failure error code is
+provided to the postrevert hook via the `rc` field of the payload structure.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+    typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
 ### .livepatch.xen_depends, .livepatch.depends and .note.gnu.build-id
 
 To support dependencies checking and safe loading (to load the
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index b3a2b9fafd..c5dae8814f 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -28,6 +28,8 @@ 
 #include <asm/alternative.h>
 #include <asm/event.h>
 
+#define is_hook_enabled(hook) ({ (hook) && *(hook); })
+
 /*
  * Protects against payload_list operations and also allows only one
  * caller in schedule_work.
@@ -501,6 +503,35 @@  static int check_special_sections(const struct livepatch_elf *elf)
     return 0;
 }
 
+/*
+ * Lookup specified section and when exists assign its address to a specified hook.
+ * Perform section pointer and size validation: single hook sections must contain a
+ * single pointer only.
+ */
+#define LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, hook, section_name) do {                        \
+    const struct livepatch_elf_sec *__sec = livepatch_elf_sec_by_name(elf, section_name); \
+    if ( !__sec )                                                                         \
+        break;                                                                            \
+    if ( !section_ok(elf, __sec, sizeof(*hook)) || __sec->sec->sh_size != sizeof(*hook) ) \
+        return -EINVAL;                                                                   \
+    hook = __sec->load_addr;                                                              \
+} while (0)
+
+/*
+ * Lookup specified section and when exists assign its address to a specified hook.
+ * Perform section pointer and size validation: multi hook sections must contain an
+ * array whose size must be a multiple of the array's items size.
+ */
+#define LIVEPATCH_ASSIGN_MULTI_HOOK(elf, hook, nhooks, section_name) do {                 \
+    const struct livepatch_elf_sec *__sec = livepatch_elf_sec_by_name(elf, section_name); \
+    if ( !__sec )                                                                         \
+        break;                                                                            \
+    if ( !section_ok(elf, __sec, sizeof(*hook)) )                                         \
+        return -EINVAL;                                                                   \
+    hook = __sec->load_addr;                                                              \
+    nhooks = __sec->sec->sh_size / sizeof(*hook);                                         \
+} while (0)
+
 static int prepare_payload(struct payload *payload,
                            struct livepatch_elf *elf)
 {
@@ -552,25 +583,14 @@  static int prepare_payload(struct payload *payload,
             return rc;
     }
 
-    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
-    if ( sec )
-    {
-        if ( !section_ok(elf, sec, sizeof(*payload->load_funcs)) )
-            return -EINVAL;
+    LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->load_funcs, payload->n_load_funcs, ".livepatch.hooks.load");
+    LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ".livepatch.hooks.unload");
 
-        payload->load_funcs = sec->load_addr;
-        payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
-    }
-
-    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
-    if ( sec )
-    {
-        if ( !section_ok(elf, sec, sizeof(*payload->unload_funcs)) )
-            return -EINVAL;
+    LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ".livepatch.hooks.preapply");
+    LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.post, ".livepatch.hooks.postapply");
+    LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.pre, ".livepatch.hooks.prerevert");
+    LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ".livepatch.hooks.postrevert");
 
-        payload->unload_funcs = sec->load_addr;
-        payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
-    }
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
     if ( sec )
     {
@@ -1217,6 +1237,39 @@  static bool_t is_work_scheduled(const struct payload *data)
     return livepatch_work.do_work && livepatch_work.data == data;
 }
 
+/*
+ * Check if payload has any of the vetoing, non-atomic hooks assigned.
+ * A vetoing, non-atmic hook may perform an operation that changes the
+ * hypervisor state and may not be guaranteed to succeed. Result of
+ * such operation may be returned and may change the livepatch workflow.
+ * Such hooks may require additional cleanup actions performed by other
+ * hooks. Thus they are not suitable for replace action.
+ */
+static inline bool_t has_payload_any_vetoing_hooks(const struct payload *payload)
+{
+    return is_hook_enabled(payload->hooks.apply.pre) ||
+           is_hook_enabled(payload->hooks.apply.post) ||
+           is_hook_enabled(payload->hooks.revert.pre) ||
+           is_hook_enabled(payload->hooks.revert.post);
+}
+
+/*
+ * Checks if any of the already applied hotpatches has any vetoing,
+ * non-atomic hooks assigned.
+ */
+static inline bool_t livepatch_applied_have_vetoing_hooks(void)
+{
+    struct payload *p;
+
+    list_for_each_entry ( p, &applied_list, applied_list )
+    {
+        if ( has_payload_any_vetoing_hooks(p) )
+            return true;
+    }
+
+    return false;
+}
+
 static int schedule_work(struct payload *data, uint32_t cmd, uint32_t timeout)
 {
     ASSERT(spin_is_locked(&payload_lock));
@@ -1317,6 +1370,7 @@  void check_for_livepatch_work(void)
     {
         struct payload *p;
         unsigned int cpus;
+        bool_t action_done = false;
 
         p = livepatch_work.data;
         if ( !get_cpu_maps() )
@@ -1369,6 +1423,7 @@  void check_for_livepatch_work(void)
             livepatch_do_action();
             /* Serialize and flush out the CPU via CPUID instruction (on x86). */
             arch_livepatch_post_action();
+            action_done = true;
             local_irq_restore(flags);
         }
 
@@ -1381,6 +1436,43 @@  void check_for_livepatch_work(void)
         /* put_cpu_maps has an barrier(). */
         put_cpu_maps();
 
+        if ( action_done )
+        {
+            switch ( livepatch_work.cmd )
+            {
+            case LIVEPATCH_ACTION_REVERT:
+                if ( is_hook_enabled(p->hooks.revert.post) )
+                {
+                    printk(XENLOG_INFO LIVEPATCH "%s: Calling post-revert hook function with rc=%d\n",
+                           p->name, p->rc);
+
+                    (*p->hooks.revert.post)(p);
+                }
+                break;
+
+            case LIVEPATCH_ACTION_APPLY:
+                if ( is_hook_enabled(p->hooks.apply.post) )
+                {
+                    printk(XENLOG_INFO LIVEPATCH "%s: Calling post-apply hook function with rc=%d\n",
+                           p->name, p->rc);
+
+                    (*p->hooks.apply.post)(p);
+                }
+                break;
+
+            case LIVEPATCH_ACTION_REPLACE:
+                if ( has_payload_any_vetoing_hooks(p) )
+                {
+                    /* It should be impossible to get here since livepatch_action() guards against that. */
+                    panic(LIVEPATCH "%s: REPLACE action is not supported on hotpatches with vetoing hooks!\n",
+                            p->name);
+                    ASSERT_UNREACHABLE();
+                }
+            default:
+                break;
+            }
+        }
+
         printk(XENLOG_INFO LIVEPATCH "%s finished %s with rc=%d\n",
                p->name, names[livepatch_work.cmd], p->rc);
     }
@@ -1516,6 +1608,21 @@  static int livepatch_action(struct xen_sysctl_livepatch_action *action)
                 rc = -EBUSY;
                 break;
             }
+
+            if ( is_hook_enabled(data->hooks.revert.pre) )
+            {
+                printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-revert hook function\n", data->name);
+
+                rc = (*data->hooks.revert.pre)(data);
+                if ( rc )
+                {
+                    printk(XENLOG_ERR LIVEPATCH "%s: pre-revert hook failed (rc=%d), aborting!\n",
+                           data->name, rc);
+                    data->rc = rc;
+                    break;
+                }
+            }
+
             data->rc = -EAGAIN;
             rc = schedule_work(data, action->cmd, action->timeout);
         }
@@ -1549,6 +1656,20 @@  static int livepatch_action(struct xen_sysctl_livepatch_action *action)
                     break;
             }
 
+            if ( is_hook_enabled(data->hooks.apply.pre) )
+            {
+                printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-apply hook function\n", data->name);
+
+                rc = (*data->hooks.apply.pre)(data);
+                if ( rc )
+                {
+                    printk(XENLOG_ERR LIVEPATCH "%s: pre-apply hook failed (rc=%d), aborting!\n",
+                           data->name, rc);
+                    data->rc = rc;
+                    break;
+                }
+            }
+
             data->rc = -EAGAIN;
             rc = schedule_work(data, action->cmd, action->timeout);
         }
@@ -1560,6 +1681,30 @@  static int livepatch_action(struct xen_sysctl_livepatch_action *action)
             rc = build_id_dep(data, 1 /* against hypervisor. */);
             if ( rc )
                 break;
+
+            /*
+             * REPLACE action is not supported on hotpatches with vetoing hooks.
+             * Vetoing hooks usually perform mutating actions on the system and
+             * typically exist in pairs (pre- hook doing an action and post- hook
+             * undoing the action). Coalescing all hooks from all applied modules
+             * cannot be performed without inspecting potential dependencies between
+             * the mutating hooks and hence cannot be performed automatically by
+             * the replace action. Also, the replace action cannot safely assume a
+             * successful revert of all the module with vetoing hooks. When one
+             * of the hooks fails due to not meeting certain conditions the whole
+             * replace operation must have been reverted with all previous pre- and
+             * post- hooks re-executed (which cannot be guaranteed to succeed).
+             * The simplest response to this complication is disallow replace
+             * action on modules with vetoing hooks.
+             */
+            if ( has_payload_any_vetoing_hooks(data) || livepatch_applied_have_vetoing_hooks() )
+            {
+                printk(XENLOG_ERR LIVEPATCH "%s: REPLACE action is not supported on hotpatches with vetoing hooks!\n",
+                       data->name);
+                rc = -EOPNOTSUPP;
+                break;
+            }
+
             data->rc = -EAGAIN;
             rc = schedule_work(data, action->cmd, action->timeout);
         }
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 99613af2db..cd20944cc4 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -21,6 +21,16 @@  typedef struct payload livepatch_payload_t;
 typedef void livepatch_loadcall_t(void);
 typedef void livepatch_unloadcall_t(void);
 
+typedef int livepatch_precall_t(livepatch_payload_t *arg);
+typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
+struct livepatch_hooks {
+    struct {
+        livepatch_precall_t *const *pre;
+        livepatch_postcall_t *const *post;
+    } apply, revert;
+};
+
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
@@ -47,6 +57,7 @@  struct payload {
     struct livepatch_build_id xen_dep;   /* ELFNOTE_DESC(.livepatch.xen_depends). */
     livepatch_loadcall_t *const *load_funcs;   /* The array of funcs to call after */
     livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
+    struct livepatch_hooks hooks;        /* Pre and post hooks for apply and revert */
     unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
     unsigned int n_unload_funcs;         /* Nr of funcs to call durung unload. */
     char name[XEN_LIVEPATCH_NAME_SIZE];  /* Name of it. */
@@ -76,6 +87,22 @@  struct payload {
      livepatch_unloadcall_t *__weak \
         const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
 
+#define LIVEPATCH_PREAPPLY_HOOK(_fn) \
+    livepatch_precall_t *__attribute__((weak, used)) \
+        const livepatch_preapply_data_##_fn __section(".livepatch.hooks.preapply") = _fn;
+
+#define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
+    livepatch_postcall_t *__attribute__((weak, used)) \
+        const livepatch_postapply_data_##_fn __section(".livepatch.hooks.postapply") = _fn;
+
+#define LIVEPATCH_PREREVERT_HOOK(_fn) \
+    livepatch_precall_t *__attribute__((weak, used)) \
+        const livepatch_prerevert_data_##_fn __section(".livepatch.hooks.prerevert") = _fn;
+
+#define LIVEPATCH_POSTREVERT_HOOK(_fn) \
+    livepatch_postcall_t *__attribute__((weak, used)) \
+        const livepatch_postrevert_data_##_fn __section(".livepatch.hooks.postrevert") = _fn;
+
 #endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
 
 /*
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 938aee17ec..a94bc48536 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -20,12 +20,16 @@  LIVEPATCH_BYE := xen_bye_world.livepatch
 LIVEPATCH_REPLACE := xen_replace_world.livepatch
 LIVEPATCH_NOP := xen_nop.livepatch
 LIVEPATCH_NO_XEN_BUILDID := xen_no_xen_buildid.livepatch
+LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
+LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
 
 LIVEPATCHES += $(LIVEPATCH)
 LIVEPATCHES += $(LIVEPATCH_BYE)
 LIVEPATCHES += $(LIVEPATCH_REPLACE)
 LIVEPATCHES += $(LIVEPATCH_NOP)
 LIVEPATCHES += $(LIVEPATCH_NO_XEN_BUILDID)
+LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
+LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
 
 LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
 
@@ -128,5 +132,18 @@  xen_no_xen_buildid.o: config.h
 $(LIVEPATCH_NO_XEN_BUILDID): xen_nop.o note.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NO_XEN_BUILDID) $^
 
+xen_prepost_hooks.o: config.h
+
+.PHONY: $(LIVEPATCH_PREPOST_HOOKS)
+$(LIVEPATCH_PREPOST_HOOKS): xen_prepost_hooks.o xen_hello_world_func.o note.o xen_note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_PREPOST_HOOKS) $^
+
+xen_prepost_hooks_fail.o: config.h
+
+.PHONY: $(LIVEPATCH_PREPOST_HOOKS_FAIL)
+$(LIVEPATCH_PREPOST_HOOKS_FAIL): xen_prepost_hooks_fail.o xen_hello_world_func.o note.o xen_note.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_PREPOST_HOOKS_FAIL) $^
+
 .PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
+           $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL)
diff --git a/xen/test/livepatch/xen_prepost_hooks.c b/xen/test/livepatch/xen_prepost_hooks.c
new file mode 100644
index 0000000000..889377d6eb
--- /dev/null
+++ b/xen/test/livepatch/xen_prepost_hooks.c
@@ -0,0 +1,122 @@ 
+/*
+ * 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 pre_apply_cnt;
+static unsigned int post_apply_cnt;
+static unsigned int pre_revert_cnt;
+static unsigned int post_revert_cnt;
+
+static unsigned int pre_revert_retry = 1;
+
+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];
+
+        pre_apply_cnt++;
+        printk(KERN_DEBUG "%s: applying: %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];
+
+        post_apply_cnt++;
+        printk(KERN_DEBUG "%s: 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];
+
+        pre_revert_cnt++;
+        printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    /* First revert attempt always fails. Second attempt succeeds. */
+    return -(pre_revert_retry--);
+}
+
+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];
+
+        post_revert_cnt++;
+        printk(KERN_DEBUG "%s: reverted: %s\n", __func__, func->name);
+    }
+
+    BUG_ON(pre_apply_cnt != 1 || post_apply_cnt != 1);
+    BUG_ON(pre_revert_cnt != 2 || post_revert_cnt != 1);
+    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_prepost_hooks_fail.c b/xen/test/livepatch/xen_prepost_hooks_fail.c
new file mode 100644
index 0000000000..87e5f6c5e1
--- /dev/null
+++ b/xen/test/livepatch/xen_prepost_hooks_fail.c
@@ -0,0 +1,75 @@ 
+/*
+ * 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);
+
+/* This hook always fail and should prevent from loading the hotpatch. */
+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];
+
+        printk(KERN_DEBUG "%s: pre applying: %s\n", __func__, func->name);
+    }
+
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return -EINVAL;
+}
+
+static int unreachable_pre_hook(livepatch_payload_t *payload)
+{
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+    BUG();
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+    return -EINVAL;
+}
+
+static void unreachable_post_hook(livepatch_payload_t *payload)
+{
+    printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+    BUG();
+    printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(unreachable_post_hook);
+LIVEPATCH_PREREVERT_HOOK(unreachable_pre_hook);
+LIVEPATCH_POSTREVERT_HOOK(unreachable_post_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:
+ */