Re: [PATCH v2] uprobes: simplify rip-relative handling

[Oleg Nesterov <oleg@redhat.com>]

On 04/28, Denys Vlasenko wrote:
>
> It is possible to replace rip-relative addressing mode
> with addressing mode of the same length: (reg+disp32).
> This eliminates the need to fix up immediate
> and instruction length.

"and change instruction length", I presume.

I like this patch very much, and I would be happy to ack everything
except this part:

> @@ -307,22 +313,13 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  		 * #1) for the scratch register.
>  		 */
>  		auprobe->def.fixups |= UPROBE_FIX_RIP_CX;
> -		/* Change modrm from 00 000 101 to 00 000 001. */
> -		*cursor = 0x1;
> +		/* Change modrm from 00 000 101 to 10 000 001. */
> +		*cursor = 0x81;
>  	} else {
>  		/* Use %rax (register #0) for the scratch register. */
>  		auprobe->def.fixups |= UPROBE_FIX_RIP_AX;
> -		/* Change modrm from 00 xxx 101 to 00 xxx 000 */
> -		*cursor = (reg << 3);
> -	}
> -
> -	/* Target address = address of next instruction + (signed) offset */
> -	auprobe->def.riprel_target = (long)insn->length + insn->displacement.value;
> -
> -	/* Displacement field is gone; slide immediate field (if any) over. */
> -	if (insn->immediate.nbytes) {
> -		cursor++;
> -		memmove(cursor, cursor + insn->displacement.nbytes, insn->immediate.nbytes);
> +		/* Change modrm from 00 xxx 101 to 10 xxx 000 */
> +		*cursor = (reg << 3) | 0x80;
>  	}

As you know, this is black magic for me.

Masami, Jim, please review and ack/nack?

> @@ -343,26 +340,17 @@ static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  		unsigned long *sr = scratch_reg(auprobe, regs);
>  
>  		utask->autask.saved_scratch_register = *sr;
> -		*sr = utask->vaddr + auprobe->def.riprel_target;
> +		*sr = utask->vaddr + (int)auprobe->def.ilen;
                                     ^^^^^

Why? This is pointless and even misleading. I'll remove this part if you
do not object.

Oleg.