Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH][v3] uprobes/x86: emulate push insns for uprobe on x86
To:     Oleg Nesterov <oleg@redhat.com>
CC:     <mingo@kernel.org>, <tglx@linutronix.de>, <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
        <netdev@vger.kernel.org>, <ast@fb.com>, <kernel-team@fb.com>
References: <20171113221139.1516536-1-yhs@fb.com>
 <20171114155124.GB17667@redhat.com>
From:   Yonghong Song <yhs@fb.com>
Message-ID: <8af0700c-dd21-8501-c40e-cea4076e80c7@fb.com>
Date:   Tue, 14 Nov 2017 14:35:23 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0)
 Gecko/20100101 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171114155124.GB17667@redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Nov 2017 22:35:26.7918 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 72fff382-0ef5-4ec6-db3b-08d52bb00145
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1834
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-14_12:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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.
> 

From 1584058403053076505@xxx Tue Nov 14 16:10:14 +0000 2017
X-GM-THRID: 1583700940287669077
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread