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

[Oleg Nesterov <oleg@redhat.com>]

Yonghong,

The patch looks good to me, but I'll try to read it carefully later.
Just a couple of cosmetic nits for now.

On 11/09, Yonghong Song wrote:
>
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -53,6 +53,10 @@ struct arch_uprobe {
>  			u8	fixups;
>  			u8	ilen;
>  		} 			defparam;
> +		struct {
> +			u8	src_offset;	/* to the start of pt_regs */
> +			u8	ilen;
> +		}			push;
>  	};
>  };

I know it is very easy to blame the naming ;) but src_offset doesn't
look nice. How about reg_offset ?

> +static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	void *src_ptr = (void *)regs + auprobe->push.src_offset;
> +
> +	if (emulate_push_stack(regs, *(unsigned long *)src_ptr))
> +		return false;

You can declare src_ptr as

	unsigned long *src_ptr = ...;

and avoid the casting below.

> +static int uprobe_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>  {
>  	u8 opc1 = OPCODE1(insn);
>  	int i;
>
>  	switch (opc1) {
> +	case 0x50 ... 0x57:
> +		return uprobe_setup_push_ops(auprobe, insn, opc1);
> +
>  	case 0xeb:	/* jmp 8 */
>  	case 0xe9:	/* jmp 32 */
>  	case 0x90:	/* prefix* + nop; same as jmp with .offs = 0 */
> @@ -767,7 +863,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (ret)
>  		return ret;
>
> -	ret = branch_setup_xol_ops(auprobe, &insn);
> +	ret = uprobe_setup_xol_ops(auprobe, &insn);
>  	if (ret != -ENOSYS)
>  		return ret;

Well... again, this is cosmetic and I won't insist, but to me it would be
more clean to not change/rename branch_setup_xol_ops(). Instead, you can
just add

	ret = uprobe_setup_push_ops(...);
	if (ret != -ENOSYS)
		return ret;

at the start of arch_uprobe_analyze_insn(), right after the similar
branch_setup_xol_ops() call.

Btw... please add v2 into the subject when you send the new version.

Oleg.