From: Mark Rutland <mark.rutland@arm.com> To: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> Cc: broonie@kernel.org, jpoimboe@redhat.com, jthierry@redhat.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v2 5/8] arm64: Detect an FTRACE frame and mark a stack trace unreliable Date: Tue, 23 Mar 2021 13:36:11 +0000 [thread overview] Message-ID: <20210323133611.GB98545@C02TD0UTHF1T.local> (raw) In-Reply-To: <2167f3c5-e7d0-40c8-99e3-ae89ceb2d60e@linux.microsoft.com> On Tue, Mar 23, 2021 at 07:56:40AM -0500, Madhavan T. Venkataraman wrote: > > > On 3/23/21 5:51 AM, Mark Rutland wrote: > > On Mon, Mar 15, 2021 at 11:57:57AM -0500, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > >> > >> When CONFIG_DYNAMIC_FTRACE_WITH_REGS is enabled and tracing is activated > >> for a function, the ftrace infrastructure is called for the function at > >> the very beginning. Ftrace creates two frames: > >> > >> - One for the traced function > >> > >> - One for the caller of the traced function > >> > >> That gives a reliable stack trace while executing in the ftrace > >> infrastructure code. When ftrace returns to the traced function, the frames > >> are popped and everything is back to normal. > >> > >> However, in cases like live patch, execution is redirected to a different > >> function when ftrace returns. A stack trace taken while still in the ftrace > >> infrastructure code will not show the target function. The target function > >> is the real function that we want to track. > >> > >> So, if an FTRACE frame is detected on the stack, just mark the stack trace > >> as unreliable. > > > > To identify this case, please identify the ftrace trampolines instead, > > e.g. ftrace_regs_caller, return_to_handler. > > > > Yes. As part of the return address checking, I will check this. IIUC, I think that > I need to check for the inner labels that are defined at the point where the > instructions are patched for ftrace. E.g., ftrace_call and ftrace_graph_call. > > SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) > bl ftrace_stub <==================================== > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); > nop <======= // If enabled, this will be replaced > // "b ftrace_graph_caller" > #endif > > For instance, the stack trace I got while tracing do_mmap() with the stack trace > tracer looks like this: > > ... > [ 338.911793] trace_function+0xc4/0x160 > [ 338.911801] function_stack_trace_call+0xac/0x130 > [ 338.911807] ftrace_graph_call+0x0/0x4 > [ 338.911813] do_mmap+0x8/0x598 > [ 338.911820] vm_mmap_pgoff+0xf4/0x188 > [ 338.911826] ksys_mmap_pgoff+0x1d8/0x220 > [ 338.911832] __arm64_sys_mmap+0x38/0x50 > [ 338.911839] el0_svc_common.constprop.0+0x70/0x1a8 > [ 338.911846] do_el0_svc+0x2c/0x98 > [ 338.911851] el0_svc+0x2c/0x70 > [ 338.911859] el0_sync_handler+0xb0/0xb8 > [ 338.911864] el0_sync+0x180/0x1c0 > > > It'd be good to check *exactly* when we need to reject, since IIUC when > > we have a graph stack entry the unwind will be correct from livepatch's > > PoV. > > > > The current unwinder already handles this like this: > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > if (tsk->ret_stack && > (ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) { > struct ftrace_ret_stack *ret_stack; > /* > * This is a case where function graph tracer has > * modified a return address (LR) in a stack frame > * to hook a function return. > * So replace it to an original value. > */ > ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++); > if (WARN_ON_ONCE(!ret_stack)) > return -EINVAL; > frame->pc = ret_stack->ret; > } > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ Beware that this handles the case where a function will return to return_to_handler, but doesn't handle unwinding from *within* return_to_handler, which we can't do reliably today, so that might need special handling. > Is there anything else that needs handling here? I wrote up a few cases to consider in: https://www.kernel.org/doc/html/latest/livepatch/reliable-stacktrace.html ... e.g. the "Obscuring of return addresses" case. It might be that we're fine so long as we refuse to unwind across exception boundaries, but it needs some thought. We probably need to go over each of the trampolines instruction-by-instruction to consider that. As mentioned above, within return_to_handler when we call ftrace_return_to_handler, there's a period where the real return address has been removed from the ftrace return stack, but hasn't yet been placed in x30, and wouldn't show up in a trace (e.g. if we could somehow hook the return from ftrace_return_to_handler). We might be saved by the fact we'll mark traces across exception boundaries as unreliable, but I haven't thought very hard about it. We might want to explciitly reject unwinds within return_to_handler in case it's possible to interpose ftrace_return_to_handler somehow. Thanks, Mark.
WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com> To: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> Cc: broonie@kernel.org, jpoimboe@redhat.com, jthierry@redhat.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v2 5/8] arm64: Detect an FTRACE frame and mark a stack trace unreliable Date: Tue, 23 Mar 2021 13:36:11 +0000 [thread overview] Message-ID: <20210323133611.GB98545@C02TD0UTHF1T.local> (raw) In-Reply-To: <2167f3c5-e7d0-40c8-99e3-ae89ceb2d60e@linux.microsoft.com> On Tue, Mar 23, 2021 at 07:56:40AM -0500, Madhavan T. Venkataraman wrote: > > > On 3/23/21 5:51 AM, Mark Rutland wrote: > > On Mon, Mar 15, 2021 at 11:57:57AM -0500, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > >> > >> When CONFIG_DYNAMIC_FTRACE_WITH_REGS is enabled and tracing is activated > >> for a function, the ftrace infrastructure is called for the function at > >> the very beginning. Ftrace creates two frames: > >> > >> - One for the traced function > >> > >> - One for the caller of the traced function > >> > >> That gives a reliable stack trace while executing in the ftrace > >> infrastructure code. When ftrace returns to the traced function, the frames > >> are popped and everything is back to normal. > >> > >> However, in cases like live patch, execution is redirected to a different > >> function when ftrace returns. A stack trace taken while still in the ftrace > >> infrastructure code will not show the target function. The target function > >> is the real function that we want to track. > >> > >> So, if an FTRACE frame is detected on the stack, just mark the stack trace > >> as unreliable. > > > > To identify this case, please identify the ftrace trampolines instead, > > e.g. ftrace_regs_caller, return_to_handler. > > > > Yes. As part of the return address checking, I will check this. IIUC, I think that > I need to check for the inner labels that are defined at the point where the > instructions are patched for ftrace. E.g., ftrace_call and ftrace_graph_call. > > SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) > bl ftrace_stub <==================================== > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); > nop <======= // If enabled, this will be replaced > // "b ftrace_graph_caller" > #endif > > For instance, the stack trace I got while tracing do_mmap() with the stack trace > tracer looks like this: > > ... > [ 338.911793] trace_function+0xc4/0x160 > [ 338.911801] function_stack_trace_call+0xac/0x130 > [ 338.911807] ftrace_graph_call+0x0/0x4 > [ 338.911813] do_mmap+0x8/0x598 > [ 338.911820] vm_mmap_pgoff+0xf4/0x188 > [ 338.911826] ksys_mmap_pgoff+0x1d8/0x220 > [ 338.911832] __arm64_sys_mmap+0x38/0x50 > [ 338.911839] el0_svc_common.constprop.0+0x70/0x1a8 > [ 338.911846] do_el0_svc+0x2c/0x98 > [ 338.911851] el0_svc+0x2c/0x70 > [ 338.911859] el0_sync_handler+0xb0/0xb8 > [ 338.911864] el0_sync+0x180/0x1c0 > > > It'd be good to check *exactly* when we need to reject, since IIUC when > > we have a graph stack entry the unwind will be correct from livepatch's > > PoV. > > > > The current unwinder already handles this like this: > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > if (tsk->ret_stack && > (ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) { > struct ftrace_ret_stack *ret_stack; > /* > * This is a case where function graph tracer has > * modified a return address (LR) in a stack frame > * to hook a function return. > * So replace it to an original value. > */ > ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++); > if (WARN_ON_ONCE(!ret_stack)) > return -EINVAL; > frame->pc = ret_stack->ret; > } > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ Beware that this handles the case where a function will return to return_to_handler, but doesn't handle unwinding from *within* return_to_handler, which we can't do reliably today, so that might need special handling. > Is there anything else that needs handling here? I wrote up a few cases to consider in: https://www.kernel.org/doc/html/latest/livepatch/reliable-stacktrace.html ... e.g. the "Obscuring of return addresses" case. It might be that we're fine so long as we refuse to unwind across exception boundaries, but it needs some thought. We probably need to go over each of the trampolines instruction-by-instruction to consider that. As mentioned above, within return_to_handler when we call ftrace_return_to_handler, there's a period where the real return address has been removed from the ftrace return stack, but hasn't yet been placed in x30, and wouldn't show up in a trace (e.g. if we could somehow hook the return from ftrace_return_to_handler). We might be saved by the fact we'll mark traces across exception boundaries as unreliable, but I haven't thought very hard about it. We might want to explciitly reject unwinds within return_to_handler in case it's possible to interpose ftrace_return_to_handler somehow. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-23 13:37 UTC|newest] Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <5997dfe8d261a3a543667b83c902883c1e4bd270> 2021-03-15 16:57 ` [RFC PATCH v2 0/8] arm64: Implement reliable stack trace madvenka 2021-03-15 16:57 ` madvenka 2021-03-15 16:57 ` [RFC PATCH v2 1/8] arm64: Implement stack trace termination record madvenka 2021-03-15 16:57 ` madvenka 2021-03-18 15:09 ` Mark Brown 2021-03-18 15:09 ` Mark Brown 2021-03-18 20:26 ` Madhavan T. Venkataraman 2021-03-18 20:26 ` Madhavan T. Venkataraman 2021-03-19 12:30 ` Mark Brown 2021-03-19 12:30 ` Mark Brown 2021-03-19 14:29 ` Madhavan T. Venkataraman 2021-03-19 14:29 ` Madhavan T. Venkataraman 2021-03-19 18:19 ` Madhavan T. Venkataraman 2021-03-19 18:19 ` Madhavan T. Venkataraman 2021-03-19 22:03 ` Madhavan T. Venkataraman 2021-03-19 22:03 ` Madhavan T. Venkataraman 2021-03-23 10:24 ` Mark Rutland 2021-03-23 10:24 ` Mark Rutland 2021-03-23 12:39 ` Madhavan T. Venkataraman 2021-03-23 12:39 ` Madhavan T. Venkataraman 2021-03-15 16:57 ` [RFC PATCH v2 2/8] arm64: Implement frame types madvenka 2021-03-15 16:57 ` madvenka 2021-03-18 17:40 ` Mark Brown 2021-03-18 17:40 ` Mark Brown 2021-03-18 22:22 ` Madhavan T. Venkataraman 2021-03-18 22:22 ` Madhavan T. Venkataraman 2021-03-19 13:22 ` Mark Brown 2021-03-19 13:22 ` Mark Brown 2021-03-19 14:40 ` Madhavan T. Venkataraman 2021-03-19 14:40 ` Madhavan T. Venkataraman 2021-03-19 15:02 ` Madhavan T. Venkataraman 2021-03-19 15:02 ` Madhavan T. Venkataraman 2021-03-19 16:20 ` Mark Brown 2021-03-19 16:20 ` Mark Brown 2021-03-19 16:27 ` Madhavan T. Venkataraman 2021-03-19 16:27 ` Madhavan T. Venkataraman 2021-03-23 10:34 ` Mark Rutland 2021-03-23 10:34 ` Mark Rutland 2021-03-15 16:57 ` [RFC PATCH v2 3/8] arm64: Terminate the stack trace at TASK_FRAME and EL0_FRAME madvenka 2021-03-15 16:57 ` madvenka 2021-03-18 18:26 ` Mark Brown 2021-03-18 18:26 ` Mark Brown 2021-03-18 20:29 ` Madhavan T. Venkataraman 2021-03-18 20:29 ` Madhavan T. Venkataraman 2021-03-23 10:36 ` Mark Rutland 2021-03-23 10:36 ` Mark Rutland 2021-03-23 12:40 ` Madhavan T. Venkataraman 2021-03-23 12:40 ` Madhavan T. Venkataraman 2021-03-15 16:57 ` [RFC PATCH v2 4/8] arm64: Detect an EL1 exception frame and mark a stack trace unreliable madvenka 2021-03-15 16:57 ` madvenka 2021-03-23 10:42 ` Mark Rutland 2021-03-23 10:42 ` Mark Rutland 2021-03-23 12:46 ` Madhavan T. Venkataraman 2021-03-23 12:46 ` Madhavan T. Venkataraman 2021-03-23 13:04 ` Mark Rutland 2021-03-23 13:04 ` Mark Rutland 2021-03-23 13:31 ` Madhavan T. Venkataraman 2021-03-23 13:31 ` Madhavan T. Venkataraman 2021-03-23 14:33 ` Mark Rutland 2021-03-23 14:33 ` Mark Rutland 2021-03-23 15:22 ` Madhavan T. Venkataraman 2021-03-23 15:22 ` Madhavan T. Venkataraman 2021-03-15 16:57 ` [RFC PATCH v2 5/8] arm64: Detect an FTRACE " madvenka 2021-03-15 16:57 ` madvenka 2021-03-23 10:51 ` Mark Rutland 2021-03-23 10:51 ` Mark Rutland 2021-03-23 12:56 ` Madhavan T. Venkataraman 2021-03-23 12:56 ` Madhavan T. Venkataraman 2021-03-23 13:36 ` Mark Rutland [this message] 2021-03-23 13:36 ` Mark Rutland 2021-03-23 13:38 ` Madhavan T. Venkataraman 2021-03-23 13:38 ` Madhavan T. Venkataraman 2021-03-23 14:15 ` Madhavan T. Venkataraman 2021-03-23 14:15 ` Madhavan T. Venkataraman 2021-03-23 14:57 ` Mark Rutland 2021-03-23 14:57 ` Mark Rutland 2021-03-23 15:26 ` Madhavan T. Venkataraman 2021-03-23 15:26 ` Madhavan T. Venkataraman 2021-03-23 16:20 ` Madhavan T. Venkataraman 2021-03-23 16:20 ` Madhavan T. Venkataraman 2021-03-23 17:02 ` Mark Rutland 2021-03-23 17:02 ` Mark Rutland 2021-03-23 17:23 ` Madhavan T. Venkataraman 2021-03-23 17:23 ` Madhavan T. Venkataraman 2021-03-23 17:27 ` Madhavan T. Venkataraman 2021-03-23 17:27 ` Madhavan T. Venkataraman 2021-03-23 18:27 ` Mark Brown 2021-03-23 18:27 ` Mark Brown 2021-03-23 20:23 ` Madhavan T. Venkataraman 2021-03-23 20:23 ` Madhavan T. Venkataraman 2021-03-23 18:30 ` Mark Rutland 2021-03-23 18:30 ` Mark Rutland 2021-03-23 20:24 ` Madhavan T. Venkataraman 2021-03-23 20:24 ` Madhavan T. Venkataraman 2021-03-23 21:04 ` Madhavan T. Venkataraman 2021-03-23 21:04 ` Madhavan T. Venkataraman 2021-03-23 16:48 ` Mark Rutland 2021-03-23 16:48 ` Mark Rutland 2021-03-23 16:53 ` Madhavan T. Venkataraman 2021-03-23 16:53 ` Madhavan T. Venkataraman 2021-03-23 17:09 ` Mark Rutland 2021-03-23 17:09 ` Mark Rutland 2021-03-15 16:57 ` [RFC PATCH v2 6/8] arm64: Check the return PC of every stack frame madvenka 2021-03-15 16:57 ` madvenka 2021-03-15 16:57 ` [RFC PATCH v2 7/8] arm64: Detect kretprobed functions in stack trace madvenka 2021-03-15 16:57 ` madvenka 2021-03-15 16:58 ` [RFC PATCH v2 8/8] arm64: Implement arch_stack_walk_reliable() madvenka 2021-03-15 16:58 ` madvenka 2021-03-15 19:01 ` [RFC PATCH v2 0/8] arm64: Implement reliable stack trace Madhavan T. Venkataraman 2021-03-15 19:01 ` Madhavan T. Venkataraman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210323133611.GB98545@C02TD0UTHF1T.local \ --to=mark.rutland@arm.com \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=jpoimboe@redhat.com \ --cc=jthierry@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=live-patching@vger.kernel.org \ --cc=madvenka@linux.microsoft.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.