lkml.org 
[lkml]   [2014]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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.


\
 
 \ /
  Last update: 2014-04-09 17:01    [W:0.138 / U:35.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site