Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' is zero.
> The purpose of this is to NOP out calls, such as:
>
>  e8 <4-bytes-offset>
>
> (5 byte insn), or on ARM a 4 byte insn for branching.
>
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
>
> If the `old_addr` is provided we will NOP 'new_size'
> amount of bytes at that location.
>
> The amount is up to 31 instructions if desired (which is
> the size of the opaque member). If there is a need to NOP
> more then: a) more 'struct livepatch_func' structures need
> to be present, b) we have to implement a variable size
> buffer (in the future), or c) first byte an unconditional
> branch skipping the to be disabled code (of course provided
> there are no branch targets in the middle).
>
> While at it, also unify the code on x86 patching so
> it is a bit simpler (instead of two seperate writes
> just make it one memcpy).
>
> And introduce a general livepatch_insn_len inline function
> that would depend on platform specific instruction size
> (for a unconditional branch). As such we also rename the
> PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
snip
> diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
> index 81f4fc9..a8e70a8 100644
> --- a/docs/misc/livepatch.markdown
> +++ b/docs/misc/livepatch.markdown
> @@ -320,10 +320,13 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
>
>  * `new_addr` is the address of the function that is replacing the old
>    function. The address is filled in during relocation. The value **MUST** be
> -  the address of the new function in the file.
> +  either the address of the new function in the file, or zero if we are NOPing out
> +  at `old_addr` (and `new_size` **MUST** not be zero).

The preceding untouched lines "...is the address of the function that is 
replacing the old function. The address is filled in during 
relocation..." are a bit stale now. Perhaps the whole paragraph needs to 
be replaced?

>
>  * `old_size` and `new_size` contain the sizes of the respective functions in bytes.
> -   The value of `old_size` **MUST** not be zero.
> +   The value of `old_size` **MUST** not be zero. If the value of `new_addr` is
> +   zero then `new_size` determines how many instruction bytes to NOP (up to
> +   opaque size modulo smallest platform instruction - 1 byte x86 and 4 bytes on ARM).

This line is probably also stale now: "...contain the sizes of the 
respective functions in bytes..."

>
>  * `version` is to be one.
>
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 05e3eb8..6eaa10f 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -101,7 +101,7 @@ static void __init arch_init_ideal_nops(void)
>  }
>
>  /* Use this to add nops to a buffer, then text_poke the whole buffer. */
> -static void init_or_livepatch add_nops(void *insns, unsigned int len)
> +void init_or_livepatch add_nops(void *insns, unsigned int len)
>  {
>      while ( len > 0 )
>      {
snip
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> -    int32_t val;
>      uint8_t *old_ptr;
> -
> -    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> -    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> +    uint8_t insn[sizeof(func->opaque)];
> +    unsigned int len;
>
>      old_ptr = func->old_addr;
> -    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> +    len = livepatch_insn_len(func);
> +    if ( !len )
> +        return;
> +
> +    memcpy(func->opaque, old_ptr, len);
> +    if ( func->new_addr )
> +    {
> +        int32_t val;
> +
> +        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> +        insn[0] = 0xe9;
> +        val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +
> +        memcpy(&insn[1], &val, sizeof(val));
> +    }
> +    else
> +        add_nops(&insn, len);

It probably doesn't make a difference, but I'd prefer this to be 
"add_nops(insn, len);".

>
> -    *old_ptr++ = 0xe9; /* Relative jump */
> -    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> -    memcpy(old_ptr, &val, sizeof(val));
> +    memcpy(old_ptr, insn, len);
>  }
>
>  void arch_livepatch_revert_jmp(const struct livepatch_func *func)
>  {
> -    memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE);
> +    memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
>  }
>

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel