diff mbox series

[livepatch-hooks-2,3/4] livepatch: Add support for apply|revert action replacement hooks

Message ID 20190814083846.89163-1-wipawel@amazon.de (mailing list archive)
State New, archived
Headers show
Series [livepatch-hooks-2,1/4] create-diff-object: Handle optional apply|revert hooks | expand

Commit Message

Wieczorkiewicz, Pawel Aug. 14, 2019, 8:38 a.m. UTC 
By default, in the quiescing zone, a hotpatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.

To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.

Since the alternative functions have direct access to the hotpatch
payload structure, they can better control context of the 'load' and
'unload' hooks execution as well as exact instructions replacement
workflows. They can be also easily extended to support extra features
in the future.

To simplify the alternative function generation move code responsible
for payload and hotpatch region registration outside of the function.
That way it is guaranteed that the registration step occurs even for
newly supplied functions.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.com>
Reviewed-by: Norbert Manthey <nmanthey@amazon.com>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.com>
---
 xen/common/livepatch.c              | 66 +++++++++++++++++++++++++++++++------
 xen/include/xen/livepatch_payload.h | 10 ++++++
 2 files changed, 66 insertions(+), 10 deletions(-)

Comments

Konrad Rzeszutek Wilk Aug. 21, 2019, 6:10 p.m. UTC | #1 
On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote:
> By default, in the quiescing zone, a hotpatch payload is applied with
> apply_payload() and reverted with revert_payload() functions. Both of
> the functions receive the payload struct pointer as a parameter. The
> functions are also a place where standard 'load' and 'unload' module
> hooks are executed.
> 
> To increase hotpatching system's agility and provide more flexiable
> long-term hotpatch solution, allow to overwrite the default apply
> and revert action functions with hook-like supplied alternatives.
> The alternative functions are optional and the default functions are
> used by default.
> 
> Since the alternative functions have direct access to the hotpatch
> payload structure, they can better control context of the 'load' and
> 'unload' hooks execution as well as exact instructions replacement
> workflows. They can be also easily extended to support extra features
> in the future.
> 
> To simplify the alternative function generation move code responsible
> for payload and hotpatch region registration outside of the function.
> That way it is guaranteed that the registration step occurs even for
> newly supplied functions.
> 

You MUST also include the test-cases for this new functionality.

Please add them, you know where they are right?
Wieczorkiewicz, Pawel Aug. 21, 2019, 6:55 p.m. UTC | #2 
On 21. Aug 2019, at 20:10, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com<mailto:konrad.wilk@oracle.com>> wrote:

On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote:
By default, in the quiescing zone, a hotpatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.

To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.

Since the alternative functions have direct access to the hotpatch
payload structure, they can better control context of the 'load' and
'unload' hooks execution as well as exact instructions replacement
workflows. They can be also easily extended to support extra features
in the future.

To simplify the alternative function generation move code responsible
for payload and hotpatch region registration outside of the function.
That way it is guaranteed that the registration step occurs even for
newly supplied functions.


You MUST also include the test-cases for this new functionality.

Yeah, I will add them. How comprehensive tests do you have in mind?
I would add 1 or 2 general cases for both of the new hooks. Is it good enough?

Please add them, you know where they are right?

I think, I do.

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
diff mbox series

Patch

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 464c07ad28..38fab8b240 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -587,8 +587,11 @@  static int prepare_payload(struct payload *payload,
     LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ".livepatch.hooks.unload");
 
     LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ".livepatch.hooks.preapply");
+    LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.action, ".livepatch.hooks.apply");
     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.action, ".livepatch.hooks.revert");
     LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ".livepatch.hooks.postrevert");
 
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
@@ -1114,6 +1117,11 @@  static int apply_payload(struct payload *data)
 
     arch_livepatch_revive();
 
+    return 0;
+}
+
+static inline void apply_payload_tail(struct payload *data)
+{
     /*
      * We need RCU variant (which has barriers) in case we crash here.
      * The applied_list is iterated by the trap code.
@@ -1121,7 +1129,7 @@  static int apply_payload(struct payload *data)
     list_add_tail_rcu(&data->applied_list, &applied_list);
     register_virtual_region(&data->region);
 
-    return 0;
+    data->state = LIVEPATCH_STATE_APPLIED;
 }
 
 static int revert_payload(struct payload *data)
@@ -1154,6 +1162,11 @@  static int revert_payload(struct payload *data)
     ASSERT(!local_irq_is_enabled());
 
     arch_livepatch_revive();
+    return 0;
+}
+
+static inline void revert_payload_tail(struct payload *data)
+{
 
     /*
      * We need RCU variant (which has barriers) in case we crash here.
@@ -1163,7 +1176,7 @@  static int revert_payload(struct payload *data)
     unregister_virtual_region(&data->region);
 
     data->reverted = true;
-    return 0;
+    data->state = LIVEPATCH_STATE_CHECKED;
 }
 
 /*
@@ -1183,15 +1196,31 @@  static void livepatch_do_action(void)
     switch ( livepatch_work.cmd )
     {
     case LIVEPATCH_ACTION_APPLY:
-        rc = apply_payload(data);
+        if ( is_hook_enabled(data->hooks.apply.action) )
+        {
+            printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook function\n", data->name);
+
+            rc = (*data->hooks.apply.action)(data);
+        }
+        else
+            rc = apply_payload(data);
+
         if ( rc == 0 )
-            data->state = LIVEPATCH_STATE_APPLIED;
+            apply_payload_tail(data);
         break;
 
     case LIVEPATCH_ACTION_REVERT:
-        rc = revert_payload(data);
+        if ( is_hook_enabled(data->hooks.revert.action) )
+        {
+            printk(XENLOG_INFO LIVEPATCH "%s: Calling revert action hook function\n", data->name);
+
+            rc = (*data->hooks.revert.action)(data);
+        }
+        else
+            rc = revert_payload(data);
+
         if ( rc == 0 )
-            data->state = LIVEPATCH_STATE_CHECKED;
+            revert_payload_tail(data);
         break;
 
     case LIVEPATCH_ACTION_REPLACE:
@@ -1202,9 +1231,18 @@  static void livepatch_do_action(void)
          */
         list_for_each_entry_safe_reverse ( other, tmp, &applied_list, applied_list )
         {
-            other->rc = revert_payload(other);
+            if ( is_hook_enabled(other->hooks.revert.action) )
+            {
+                printk(XENLOG_INFO LIVEPATCH "%s: Calling revert action hook function\n", other->name);
+
+                other->rc = (*other->hooks.revert.action)(other);
+            }
+            else
+                other->rc = revert_payload(other);
+
+
             if ( other->rc == 0 )
-                other->state = LIVEPATCH_STATE_CHECKED;
+                revert_payload_tail(other);
             else
             {
                 rc = -EINVAL;
@@ -1214,9 +1252,17 @@  static void livepatch_do_action(void)
 
         if ( rc == 0 )
         {
-            rc = apply_payload(data);
+            if ( is_hook_enabled(data->hooks.apply.action) )
+            {
+                printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook function\n", data->name);
+
+                rc = (*data->hooks.apply.action)(data);
+            }
+            else
+                rc = apply_payload(data);
+
             if ( rc == 0 )
-                data->state = LIVEPATCH_STATE_APPLIED;
+                apply_payload_tail(data);
         }
         break;
 
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index cd20944cc4..ff16af0dd6 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -22,11 +22,13 @@  typedef void livepatch_loadcall_t(void);
 typedef void livepatch_unloadcall_t(void);
 
 typedef int livepatch_precall_t(livepatch_payload_t *arg);
+typedef int livepatch_actioncall_t(livepatch_payload_t *arg);
 typedef void livepatch_postcall_t(livepatch_payload_t *arg);
 
 struct livepatch_hooks {
     struct {
         livepatch_precall_t *const *pre;
+        livepatch_actioncall_t *const *action;
         livepatch_postcall_t *const *post;
     } apply, revert;
 };
@@ -91,6 +93,10 @@  struct payload {
     livepatch_precall_t *__attribute__((weak, used)) \
         const livepatch_preapply_data_##_fn __section(".livepatch.hooks.preapply") = _fn;
 
+#define LIVEPATCH_APPLY_HOOK(_fn) \
+    livepatch_actioncall_t *__attribute__((weak, used)) \
+        const livepatch_apply_data_##_fn __section(".livepatch.hooks.apply") = _fn;
+
 #define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
     livepatch_postcall_t *__attribute__((weak, used)) \
         const livepatch_postapply_data_##_fn __section(".livepatch.hooks.postapply") = _fn;
@@ -99,6 +105,10 @@  struct payload {
     livepatch_precall_t *__attribute__((weak, used)) \
         const livepatch_prerevert_data_##_fn __section(".livepatch.hooks.prerevert") = _fn;
 
+#define LIVEPATCH_REVERT_HOOK(_fn) \
+    livepatch_actioncall_t *__attribute__((weak, used)) \
+        const livepatch_revert_data_##_fn __section(".livepatch.hooks.revert") = _fn;
+
 #define LIVEPATCH_POSTREVERT_HOOK(_fn) \
     livepatch_postcall_t *__attribute__((weak, used)) \
         const livepatch_postrevert_data_##_fn __section(".livepatch.hooks.postrevert") = _fn;