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

Re: [Xen-devel] [PATCH v3 04/12] livepatch: Implement pre-|post- apply|revert hooks



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,
--
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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