On Sun, 2014-04-06 at 22:16 +0200, Oleg Nesterov wrote:
> 0xeb and 0xe9. Anything else?
For unconditional rip-relative jumps, yes, I'm pretty sure that's it.
>
> TODO: move ->fixup into the union along with rip_rela_target_address.
>
> Reported-by: Jonathan Lebon <jle...@redhat.com>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
> arch/x86/include/asm/uprobes.h | 8 +++++++-
> arch/x86/kernel/uprobes.c | 40
> ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 9f8210b..cca62c5 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -44,9 +44,15 @@ struct arch_uprobe {
> u16 fixups;
> const struct uprobe_xol_ops *ops;
>
> + union {
> #ifdef CONFIG_X86_64
> - unsigned long rip_rela_target_address;
> + unsigned long rip_rela_target_address;
> #endif
> + struct {
> + s32 disp;
> + u8 ilen;
> + } ttt;
Are you planning to stick with ttt as the name/prefix for all this
jump-emulation code? Seems like you could come up with something more
descriptive without adding a lot of characters.
> + };
> };
>
> struct arch_uprobe_task {
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 1ea7e1a..32ab147 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -466,6 +466,42 @@ static struct uprobe_xol_ops default_xol_ops = {
> .post_xol = default_post_xol_op,
> };
>
> +static bool ttt_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> + regs->ip += auprobe->ttt.ilen + auprobe->ttt.disp;
> + return true;
> +}
> +
> +static struct uprobe_xol_ops ttt_xol_ops = {
> + .emulate = ttt_emulate_op,
> +};
> +
> +static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> +{
> +
> + switch (OPCODE1(insn)) {
> + case 0xeb: /* jmp 8 */
> + case 0xe9: /* jmp 32 */
> + break;
> + default:
> + return -ENOSYS;
-ENOSYS looks like an error return, as opposed to what it is, the normal
return when the probed instruction is something other than a jump. This
gets more bewildering as you add patches and this switch grows and gets
more complicated. Add a comment here?
> + }
> +
> + /* has the side-effect of processing the entire instruction */
> + insn_get_length(insn);
> + if (WARN_ON_ONCE(!insn_complete(insn)))
> + return -ENOEXEC;
> +
> + auprobe->ttt.ilen = insn->length;
> + auprobe->ttt.disp = insn->moffset1.value;
> + /* so far we assume that it fits into ->moffset1 */
> + if (WARN_ON_ONCE(insn->moffset2.nbytes))
> + return -ENOEXEC;
s/moffset1/immediate/ -- which you've already addressed.
> +
> + auprobe->ops = &ttt_xol_ops;
> + return 0;
> +}
> +
> /**
> * arch_uprobe_analyze_insn - instruction analysis including validity and
> fixups.
> * @mm: the probed address space.
> @@ -483,6 +519,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe
> *auprobe, struct mm_struct *mm,
> if (ret)
> return ret;
>
> + ret = ttt_setup_xol_ops(auprobe, &insn);
> + if (ret == 0 || ret != ENOSYS)
This looks wrong in a couple of ways:
a. I think you intend to compare against -ENOSYS, not ENOSYS.
b. Given the (ret != [-]ENOSYS) test, the (ret == 0) test is
superfluous.
> + return ret;
> +
> /*
> * Figure out which fixups arch_uprobe_post_xol() will need to perform,
> * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups
Jim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
