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.
--
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/
