Commit 8a6b1732 authored Mar 30, 2014 by Oleg Nesterov

uprobes: Kill UPROBE_SKIP_SSTEP and can_skip_sstep()



UPROBE_COPY_INSN, UPROBE_SKIP_SSTEP, and uprobe->flags must die. This
patch kills UPROBE_SKIP_SSTEP. I never understood why it was added;
not only it doesn't help, it harms.

It can only help to avoid arch_uprobe_skip_sstep() if it was already
called before and failed. But this is ugly, if we want to know whether
we can emulate this instruction or not we should do this analysis in
arch_uprobe_analyze_insn(), not when we hit this probe for the first
time.

And in fact this logic is simply wrong. arch_uprobe_skip_sstep() can
fail or not depending on the task/register state, if this insn can be
emulated but, say, put_user() fails we need to xol it this time, but
this doesn't mean we shouldn't try to emulate it when this or another
thread hits this bp next time.

And this is the actual reason for this change. We need to emulate the
"call" insn, but push(return-address) can obviously fail.

Per-arch notes:

	x86: __skip_sstep() can only emulate "rep;nop". With this
	     change it will be called every time and most probably
	     for no reason.

	     This will be fixed by the next changes. We need to
	     change this suboptimal code anyway.

	arm: Should not be affected. It has its own "bool simulate"
	     flag checked in arch_uprobe_skip_sstep().

	ppc: Looks like, it can emulate almost everything. Does it
	     actually need to record the fact that emulate_step()
	     failed? Hopefully not. But if yes, it can add the ppc-
	     specific flag into arch_uprobe.

TODO: rename arch_uprobe_skip_sstep() to arch_uprobe_emulate_insn(),

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reviewed-by: David A. Long <dave.long@linaro.org>
Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

parent b3d5fc3c

kernel/events/uprobes.c

+2 −21

Original line number	Diff line number	Diff line
		@@ -60,8 +60,6 @@ static struct percpu_rw_semaphore dup_mmap_sem;

		/* Have a copy of original instruction */
		#define UPROBE_COPY_INSN 0
		/* Can skip singlestep */
		#define UPROBE_SKIP_SSTEP 1

		struct uprobe {
		struct rb_node rb_node; /* node in the rb tree */
		@@ -491,12 +489,9 @@ static struct uprobe alloc_uprobe(struct inode inode, loff_t offset)
		uprobe->offset = offset;
		init_rwsem(&uprobe->register_rwsem);
		init_rwsem(&uprobe->consumer_rwsem);
		/* For now assume that the instruction need not be single-stepped */
		__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);

		/* add to uprobes_tree, sorted on inode:offset */
		cur_uprobe = insert_uprobe(uprobe);

		/* a uprobe exists for this inode:offset combination */
		if (cur_uprobe) {
		kfree(uprobe);
		@@ -1628,20 +1623,6 @@ bool uprobe_deny_signal(void)
		return true;
		}

		/*
		* Avoid singlestepping the original instruction if the original instruction
		* is a NOP or can be emulated.
		*/
		static bool can_skip_sstep(struct uprobe uprobe, struct pt_regs regs)
		{
		if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
		return true;
		clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
		}
		return false;
		}

		static void mmf_recalc_uprobes(struct mm_struct *mm)
		{
		struct vm_area_struct *vma;
		@@ -1868,13 +1849,13 @@ static void handle_swbp(struct pt_regs *regs)

		handler_chain(uprobe, regs);

		if (can_skip_sstep(uprobe, regs))
		if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
		goto out;

		if (!pre_ssout(uprobe, regs, bp_vaddr))
		return;

		/* can_skip_sstep() succeeded, or restart if can't singlestep */
		/* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */
		out:
		put_uprobe(uprobe);
		}

Admin message