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

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 16. Sep 2019, at 19:54, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> 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


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