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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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