| Message ID | 20160919170233.GC30790@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.09.16 at 19:02, <konrad.wilk@oracle.com> wrote: > On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote: >> >>> On 19.09.16 at 18:11, <konrad.wilk@oracle.com> wrote: >> > 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; >> > } >> >> 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. > > Uh-uh. > > The _new_size_ determines how many bytes to NOP (in the context of > this patch). The old_size (where we check to be at min 2) is a safety > valve to make sure we don't NOP something outside the function. Well, all this looks a little fishy to me: I don't see the relation to functions at all here. Patching can be done anywhere - at the start of a function, in its middle, at the end, or - in extreme cases - even spanning function boundaries. So perhaps all you really want then (without altering that basic concept) is new_size <= old_size? >> >> 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? > > It expands the existing ones. Right now in 'staging' branch we have an > arch/arm/livepatch.c which has these functions in it. > > Granted nothing compiles them, so I could break it in this patch. > > But I already cobbled up the patch so may as well use it? Oh, sure. Jan
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index 755f596..7f067a0 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) return -ENOSYS; } -void arch_livepatch_apply_jmp(struct livepatch_func *func) +void arch_livepatch_apply(struct livepatch_func *func) { } -void arch_livepatch_revert_jmp(const struct livepatch_func *func) +void arch_livepatch_revert(const struct livepatch_func *func) { } diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 118770e..36bbc5f 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -47,7 +47,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) return 0; } -void arch_livepatch_apply_jmp(struct livepatch_func *func) +void arch_livepatch_apply(struct livepatch_func *func) { uint8_t *old_ptr; uint8_t insn[sizeof(func->opaque)]; @@ -76,7 +76,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func) memcpy(old_ptr, insn, len); } -void arch_livepatch_revert_jmp(const struct livepatch_func *func) +void arch_livepatch_revert(const struct livepatch_func *func) { memcpy(func->old_addr, func->opaque, livepatch_insn_len(func)); } diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 174af06..b7f66d4 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -86,8 +86,8 @@ unsigned int livepatch_insn_len(const struct livepatch_func *func) int arch_livepatch_quiesce(void); void arch_livepatch_revive(void); -void arch_livepatch_apply_jmp(struct livepatch_func *func); -void arch_livepatch_revert_jmp(const struct livepatch_func *func); +void arch_livepatch_apply(struct livepatch_func *func); +void arch_livepatch_revert(const struct livepatch_func *func); void arch_livepatch_post_action(void); void arch_livepatch_mask(void);