Re: [PATCH] uprobes/x86: emulate push insns for uprobe on x86

On 11/9/17 5:44 AM, Oleg Nesterov wrote:

On 11/09, Yonghong Song wrote:

quoted

This patch extends the emulation to "push <reg>"
insns. These insns are typical in the beginning
of the function. For example, bcc
in https://github.com/iovisor/bcc repo provides
tools to measure funclantency, detect memleak, etc.
The tools will place uprobes in the beginning of
function and possibly uretprobes at the end of function.
This patch is able to reduce the trap overhead for
uprobe from 2 to 1.

OK. but to be honest I do not like the implementation, please see below.

quoted

+enum uprobe_insn_t {
+	UPROBE_BRANCH_INSN	= 0,
+	UPROBE_PUSH_INSN	= 1,
+};
+
  struct uprobe_xol_ops;

  struct arch_uprobe {

@@ -42,6 +47,7 @@ struct arch_uprobe {
  	};

  	const struct uprobe_xol_ops	*ops;
+	enum uprobe_insn_t		insn_class;

Why?

I'd suggest to leave branch_xol_ops alone and add the new push_xol_ops{},
the code will look much simpler.

The only thing they can share is branch_post_xol_op() which is just

	regs->sp += sizeof_long();
	return -ERESTART;

I think a bit of code duplication would be fine in this case.

Just prototyped. Agreed, having seperate uprobe_xol_ops for "push" 
emulation is clean and better.

And. Do you really need ->post_xol() method to emulate "push"? Why we can't
simply execute it out-of-line if copy_to_user() fails?

Thanks for pointing it out. Agreed, we do not really need post_xol for 
"push". xol execution is just fine.

Will address your other comments as well in the next revision.

branch_post_xol_op() is needed because we can't execute "call" out-of-line,
we need to restart and try again if copy_to_user() fails, but I don not
understand why it is needed to emulate "push".

Oleg.