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