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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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).

> +{
> +    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.
> +        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

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