Re: [PATCH 2/5] uprobes: introduce uprobe_switch_to()

[Oleg Nesterov]

On 11/30, Peter Zijlstra wrote:
>
> On Tue, 2011-11-29 at 18:18 +0100, Oleg Nesterov wrote:
> > On 11/28, Peter Zijlstra wrote:
> > >
> > > So uprobe_switch_to() will always reset the IP to the start of the slot?
> > > That sounds wrong, things like the RIP relative stuff needs multiple
> > > instructions.
> >
> > Hmm. Could you explain? Especially the "multiple instructions" part.
> >
> > In any case we should reset the IP to the start of the slot.
> >
> > But yes, I'm afraid this is too simple. Before this patches pre_xol()
> > is called when we already know ->xol_vaddr. But afaics x86 doesn't use
> > this info (post_xol() does). So this looks equally correct or wrong.
> >
> > But perhaps we need another arch-dependent hook which takes ->xol_vaddr
> > into account instead of simple memcpy(), to handle the RIP relative
> > case.
> >
> > Or I misunderstood?
>
> Suppose you need multiple instructions to replace the one you patched
> out,

Ah, I see, thanks...

Yes, in this case set_xol_ip() should add the offset,
regs->ip % UPROBES_XOL_SLOT_BYTES.

But the current code doesn't use multiple instructions and it relies
on the single-stepping, so I think currently this is correct.

> for example because the instruction was RIP relative (the effect
> relied on the IP the instruction is at, eg. short jumps instead of
> absolute jumps).
>
> One way to translate these instructions is something like
>
>   push eax
>   mov eax, $previous_ip
>   $ins eax+offset
>   pop eax

I can be easily wrong, but afaics this particular case is covered by
pre_xol/post_xol. But I guess this doesn't matter.

Yes, I thought about multiple insns in xol slot too.

> Also, the thing Srikar mentioned is boosted probes, in that case you
> forgo the whole single step thing and rewrite the probe as:
>
>   $ins
>   jmp $next_insn

Yes! it would be nice to avoid the stepping if possible. But so far
I am not sure how/when this can work...

> Now in the former case you still single step so the context switch hook
> can function as proposed (triggered off of TIF_SINGLESTEP). However if
> you get preempted after the mov you want to continue with the $ins, not
> restart at push.

This is not clear to me. Single step with multiple insns?

> So uprobe_switch_to() will have to preserve the
> relative offset within the slot.

Yes, agreed.

> On the second example there's no singlestepping left, so we need to
> create a new TIF flag, when you first set up the probe you toggle that
> flag and on the first context switch where the IP is outside of the slot
> you clear it. But still you need to maintain relative offset within the
> slot when you move it around.

Yes. Currently uprobe_switch_to() checks X86_EFLAGS_TF() to verify that
it is correct to change regs->ip. But if we know that, say, this insn
can't jump/call/rep we can simply check regs->ip. And in this case we
can avoid the stepping.

Thanks,

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/