From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934140AbdJQIGR (ORCPT ); Tue, 17 Oct 2017 04:06:17 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:56656 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933030AbdJQIGJ (ORCPT ); Tue, 17 Oct 2017 04:06:09 -0400 Date: Tue, 17 Oct 2017 13:35:59 +0530 From: "Naveen N. Rao" To: Masami Hiramatsu Cc: Ingo Molnar , mingo@redhat.com, x86@kernel.org, Steven Rostedt , linux-kernel@vger.kernel.org, Peter Zijlstra , Ananth N Mavinakayanahalli , Thomas Gleixner , "H . Peter Anvin" , "Paul E . McKenney" , Alexei Starovoitov , Alexei Starovoitov , Michael Ellerman Subject: Re: [PATCH -tip v3 3/7] kprobes: Warn if optprobe handler tries to change execution path References: <150581509713.32348.1905525476438163954.stgit@devbox> <150581521955.32348.3615624715034787365.stgit@devbox> <20171010170231.pa6d25rbcw25ln3l@naverao1-tp.localdomain> <20171012140409.afee6696cff6c546ce7c4351@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171012140409.afee6696cff6c546ce7c4351@kernel.org> User-Agent: NeoMutt/20170912 (1.9.0) X-TM-AS-MML: disable x-cbid: 17101708-0040-0000-0000-000003E38803 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17101708-0041-0000-0000-000025E5D07A Message-Id: <20171017080559.3exsw66lmetpxg2i@naverao1-tp.localdomain> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-17_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710170113 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/10/12 05:04AM, Masami Hiramatsu wrote: > On Tue, 10 Oct 2017 22:32:31 +0530 > "Naveen N. Rao" wrote: > > > On 2017/09/19 10:00AM, Masami Hiramatsu wrote: > > So, we don't seem to _require_ users to return !0 if the handler > > changes [n]ip? Or to always change [n]ip if returning !0. > > > > The implicit assumption seems to be that the handler returns !0 if it > > wants to suppress executing the probed instruction since the handler has > > already taken care of that. So, at the least, I think the message should > > change. However... > > > > In powerpc, we place a probe on kretprobe_trampoline and optimize it. > > Oh, what did you do?? I think kretprobe_trampoline just calls > its handler to get correct address to return and just return to it. For x86 yes, but on powerpc, we use the original implementation of placing a probe at kretprobe_trampoline for catching the function return. > > > This works for us (even though optprobes doesn't "honour" changes to > > [n]ip). See commit 762df10bad6954 ("powerpc/kprobes: Optimize kprobe in > > kretprobe_trampoline()"). With this patch, we are now seeing a warning > > (thanks to mpe for the report): > > > > [ 520.144449] ------------[ cut here ]------------ > > [ 520.144676] WARNING: CPU: 2 PID: 6355 at kernel/kprobes.c:391 opt_pre_handler+0xe8/0x110 > > ... > > [ 520.151806] CPU: 2 PID: 6355 Comm: ftracetest Not tainted 4.14.0-rc4-gcc6-next-20171009-g49827b9 #1 > > [ 520.152097] task: c0000000e9ddfb80 task.stack: c0000000f881c000 > > [ 520.152291] NIP: c0000000001f3b78 LR: c0000000001f3b2c CTR: > > c0000000002436a0 > > [ 520.152527] REGS: c0000000f881f7f0 TRAP: 0700 Not tainted (4.14.0-rc4-gcc6-next-20171009-g49827b9) > > [ 520.152818] MSR: 8000000100021033 CR: 24002824 XER: 20000000 > > [ 520.153080] CFAR: c0000000001f3b34 SOFTE: 0 > > ... > > [ 520.155113] NIP [c0000000001f3b78] opt_pre_handler+0xe8/0x110 > > [ 520.155320] LR [c0000000001f3b2c] opt_pre_handler+0x9c/0x110 > > [ 520.155510] Call Trace: > > [ 520.155590] [c0000000f881fa70] [c0000000001f3b2c] opt_pre_handler+0x9c/0x110 (unreliable) > > [ 520.155825] [c0000000f881fb00] [c000000000047de8] optimized_callback+0xc8/0xe0 > > [ 520.156047] [c0000000f881fb40] [c000000000048764] optinsn_slot+0xec/0x10000 > > [ 520.156238] [c0000000f881fe30] [c000000000046cb0] kretprobe_trampoline+0x0/0x10 > > [ 520.156452] Instruction dump: > > [ 520.156570] 7fbef840 409effa4 38210090 e8010010 eb41ffd0 eb61ffd8 eb81ffe0 eba1ffe8 > > [ 520.156792] ebc1fff0 ebe1fff8 7c0803a6 4e800020 <0fe00000> e89e0028 3c62ffce 386362b0 > > [ 520.157016] ---[ end trace d8cda029528a560d ]--- > > [ 520.157172] Optprobe ignores instruction pointer changing.(kretprobe_trampoline+0x0/0x10) > > > > > > So, should this patch be reverted? > > Hmm, I got it. It seems to depend on arch implementation. Yes, we're optimizing the probe at kretprobe_trampoline, so we need this. > Anyway, this is just adding an warning, we can safely revert it. > And the documentation should be updated. > > Ingo, could you revert this change? Thanks! I will send a patch to revert this change. - Naveen