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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
> >>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> 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

The original intent behind .old_size was to guard against patching
functions that were less than our relative jump. 

(The tools end up computing the .old_size as the size of the whole function
which is fine).

But with this NOPing support, you are right - we could have now an
function that is say 4 bytes long and we only need to NOP three bytes
out of it (the last instruction I assume would be 'ret').

So perhaps this check needs just needs an 'else if' , like so:

int arch_livepatch_verify_func(const struct livepatch_func *func)
{
    /* If NOPing.. */
    if ( !func->new_addr )
    {
        /* Only do up to maximum amount we can put in the ->opaque. */
        if ( func->new_size > sizeof(func->opaque) )
            return -EOPNOTSUPP;

        /* One instruction for 'ret' and the other to NOP. */
        if ( func->old_size < 2 )
            return -EINVAL;
    }
    else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
        return -EINVAL;

    return 0;
}

[And update the design]
> 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

Not at all. Just missed it.
> NOP addition here, perhaps worth dropping the _jmp from the
> function name (and its revert counterpart)?

Ooh, good idea. But I think it maybe better as a seperate patch (as it
also touches the ARM code).

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

/me nods.

> 
> Jan
> 

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