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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.16 at 17:29, <konrad.wilk@xxxxxxxxxx> wrote:
> @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>  
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> -    /* No NOP patching yet. */
> -    if ( !func->new_size )
> +    /* If NOPing only do up to maximum amount we can put in the ->opaque. */
> +    if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>          return -EOPNOTSUPP;
>  
> -    if ( func->old_size < PATCH_INSN_SIZE )
> +    if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>          return -EINVAL;

Is that indeed a requirement when NOPing? You can easily NOP out
just a single byte on x86. Or shouldn't in that case old_size == new_size
anyway? In which case the comment further down stating that new_size
can be zero would also be wrong.

> @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct 
> livepatch_func *func)
>  
>  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);
>  
> -    *old_ptr++ = 0xe9; /* Relative jump */

Are you btw intentionally getting rid of this comment? And with the
NOP addition here, perhaps worth dropping the _jmp from the
function name (and its revert counterpart)?

> +static inline size_t livepatch_insn_len(const struct livepatch_func *func)

I think it would be nice to use consistent types: The current sole caller
stores the result of the function in an unsigned int, and I see no reason
why the function couldn't also return such.

Jan


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