Messages in this thread Patch in this message | | | Date | Wed, 9 Apr 2014 16:47:05 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC PATCH 1/6] uprobes/x86: Emulate unconditional rip-relative jmp's |
| |
On 04/08, Jim Keniston wrote: > > 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.
Great, thanks.
> > --- 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.
Yes sure. How about s/ttt/branch/ ? I agree with any naming. I used "ttt" because it allows me to change the naming in one step.
> > +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?
OK, I added a short comment above this function,
/* Returns -ENOSYS if ttt_xol_ops doesn't handle this insn */ static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) ...
> > + /* 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.
Yes, dons, and I removed the ->moffset2 check.
> > @@ -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.
OOPS! fixed.
> b. Given the (ret != [-]ENOSYS) test, the (ret == 0) test is > superfluous.
I thought that the additional "ret == 0" (removed by gcc anyway) could help the code reader... But yes, you are right, probably it only adds more confusion.
- if (ret == 0 || ret != ENOSYS) + if (ret != -ENOSYS)
Thanks!
Oleg.
| |