Re: [Xen-devel] [PATCH v3 07/12] livepatch: Add per-function applied/reverted state tracking marker

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

> On 19. Sep 2019, at 17:18, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
> 
> On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
>> Livepatch only tracks an entire payload applied/reverted state. But,
>> with an option to supply the apply_payload() and/or revert_payload()
>> functions as optional hooks, it becomes possible to intermix the
>> execution of the original apply_payload()/revert_payload() functions
>> with their dynamically supplied counterparts.
>> It is important then to track the current state of every function
>> being patched and prevent situations of unintentional double-apply
>> or unapplied revert.
>> To support that, it is necessary to extend public interface of the
>> livepatch. The struct livepatch_func gets additional field holding
>> the applied/reverted state marker.
>> To reflect the livepatch payload ABI change, bump the version flag
>> LIVEPATCH_PAYLOAD_VERSION up to 2.
>> [And also update the top of the design document]
> snip> @@ -834,6 +839,8 @@ struct livepatch_func {
>>      uint32_t old_size;
>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>      uint8_t opaque[31];
>> +    uint8_t applied;
>> +    uint8_t _pad[7];
>>  };
>>  typedef struct livepatch_func livepatch_func_t;
>>  #endif
>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
>> index 2aec532ee2..28f9536776 100644
>> --- a/xen/include/xen/livepatch.h
>> +++ b/xen/include/xen/livepatch.h
>> @@ -109,6 +109,31 @@ static inline int livepatch_verify_distance(const 
>> struct livepatch_func *func)
>>        return 0;
>>  }
>> +
>> +static inline bool_t is_func_applied(const struct livepatch_func *func)
> 
> Use bool rather than bool_t (throughout the patch).
> 

ACK.

>> +{
>> +    if ( func->applied == LIVEPATCH_FUNC_APPLIED )
>> +    {
>> +        printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied 
>> before\n",
>> +                __func__, func->name);
> 
> How about dropping this function and having a wrapper function like this:
> 
> common_livepatch_apply() {
>    if (func->applied == LIVEPATCH_FUNC_APPLIED) {
>        WARN(...)
>        return
>    }
> 
>    arch_livepatch_apply()
> 
>    func->applied = LIVEPATCH_FUNC_APPLIED
> }
> 
> This could be used by the normal apply code and any apply hooks.
> 
> This avoids having duplicate code in each of the architectures that is not 
> arch specific and also avoids having a state querying function emit a warning 
> which seems odd to me.
> 

Yes. That makes a lot of sense. Let me do that.

Thanks.

>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static inline bool_t is_func_reverted(const struct livepatch_func *func)
>> +{
>> +    if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED )
>> +    {
>> +        printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied 
>> before\n",
>> +                __func__, func->name);
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>  /*
>>   * These functions are called around the critical region patching live code,
>>   * for an architecture to take make appropratie global state adjustments.
>> @@ -117,7 +142,7 @@ int arch_livepatch_quiesce(void);
>>  void arch_livepatch_revive(void);
>>  
> -- 
> 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