linux-kernel - Re: [PATCH 2/5] uprobes: introduce uprobe_switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Nov 2011 18:10:29 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-mm <linux-mm@...ck.org>, Ingo Molnar <mingo@...e.hu>,
	Andi Kleen <andi@...stfloor.org>,
	Christoph Hellwig <hch@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Roland McGrath <roland@...k.frob.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Anton Arapov <anton@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Stephen Wilson <wilsons@...rt.ca>
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.

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