Messages in this thread | | | Date | Wed, 30 Nov 2011 18:10:29 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/5] uprobes: introduce uprobe_switch_to() |
| |
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.
| |