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

[Yonghong Song]

On 11/14/17 7:51 AM, Oleg Nesterov wrote:
> On 11/13, Yonghong Song wrote:
> > +static int push_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> > +{
> > +	u8 opc1 = OPCODE1(insn), reg_offset = 0;
> > +
> > +	if (opc1 < 0x50 || opc1 > 0x57)
> > +		return -ENOSYS;
> > +
> > +	if (insn->length > 2)
> > +		return -ENOSYS;
> > +#ifdef CONFIG_X86_64
> > +	if (test_thread_flag(TIF_ADDR32))
> > +		return -ENOSYS;
> > +#endif
>
> No, this doesn't look right, see my previous email. You should do this
> check in the "if (insn->length == 2)" branch below, "push bp" should be
> emulated correctly.
>
> And test_thread_flag(TIF_ADDR32) is not right too. The caller is not
> necessarily the probed task. See is_64bit_mm(mm) in arch_uprobe_analyze_insn().

I printed out some statistics. On x86_64 platform, for 32bit 
application, test_thread_flag(TIF_ADDR32) returns true and 
is_64bit_mm(mm) returns false. For 64bit application,
test_thread_flag(TIF_ADDR32) returns false and is_64bit_mm(mm) return 
true. So that is why my patch works fine.

I did not fully understand how to trigger "the caller is not necessarily 
the probed task." So in the next revision, I will use is_64bit_mm(mm) 
instead.

> And again... please check if uprobe_init_insn() fails or not in this case
> (32bit task does, say, "push r8"). If it fails, your V2 should be fine.

The compiler won't generated "push r8" for 32bit task since register 
"r8" is not available on 32bit instruction.

> To remind, uprobes && 32-bit is broken, let me quote my another email:
>
> 	The 3rd bug means that you simply can't uprobe a 32bit task on a 64bit
> 	system, the in_compat_syscall() logic in get_unmapped_area() looks very
> 	wrong although I need to re-check.
>
> I didn't have time for this problem so far. But emulation should work, so
> you can hopefully test your patch.
>
> Oleg.