>>> On 19.09.16 at 18:11, <konrad.w...@oracle.com> wrote: > 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; > }
Except that I wouldn't use 2, to not exclude patching out some single byte in the middle of a function, without regard to what the function's actual size is. >> 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). That's in the other series, isn't it? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel