From: Michael Ellerman <mpe@ellerman.id.au> To: Nicolai Stange <nstange@suse.de> Cc: Joe Lawrence <joe.lawrence@redhat.com>, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, live-patching@vger.kernel.org, Balbir Singh <bsingharora@gmail.com>, Jiri Kosina <jkosina@suse.cz>, Josh Poimboeuf <jpoimboe@redhat.com>, Nicolai Stange <nstange@suse.de>, Torsten Duwe <duwe@lst.de> Subject: Re: [PATCH 1/4] powerpc/64s: Clear on-stack exception marker upon exception return Date: Thu, 31 Jan 2019 16:46:05 +1100 [thread overview] Message-ID: <87zhrhnzvm.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <87a7jirrn6.fsf@suse.de> Nicolai Stange <nstange@suse.de> writes: > Michael Ellerman <mpe@ellerman.id.au> writes: > >> Joe Lawrence <joe.lawrence@redhat.com> writes: >>> From: Nicolai Stange <nstange@suse.de> >>> >>> The ppc64 specific implementation of the reliable stacktracer, >>> save_stack_trace_tsk_reliable(), bails out and reports an "unreliable >>> trace" whenever it finds an exception frame on the stack. Stack frames >>> are classified as exception frames if the STACK_FRAME_REGS_MARKER magic, >>> as written by exception prologues, is found at a particular location. >>> >>> However, as observed by Joe Lawrence, it is possible in practice that >>> non-exception stack frames can alias with prior exception frames and thus, >>> that the reliable stacktracer can find a stale STACK_FRAME_REGS_MARKER on >>> the stack. It in turn falsely reports an unreliable stacktrace and blocks >>> any live patching transition to finish. Said condition lasts until the >>> stack frame is overwritten/initialized by function call or other means. >>> >>> In principle, we could mitigate this by making the exception frame >>> classification condition in save_stack_trace_tsk_reliable() stronger: >>> in addition to testing for STACK_FRAME_REGS_MARKER, we could also take into >>> account that for all exceptions executing on the kernel stack >>> - their stack frames's backlink pointers always match what is saved >>> in their pt_regs instance's ->gpr[1] slot and that >>> - their exception frame size equals STACK_INT_FRAME_SIZE, a value >>> uncommonly large for non-exception frames. >>> >>> However, while these are currently true, relying on them would make the >>> reliable stacktrace implementation more sensitive towards future changes in >>> the exception entry code. Note that false negatives, i.e. not detecting >>> exception frames, would silently break the live patching consistency model. >>> >>> Furthermore, certain other places (diagnostic stacktraces, perf, xmon) >>> rely on STACK_FRAME_REGS_MARKER as well. >>> >>> Make the exception exit code clear the on-stack STACK_FRAME_REGS_MARKER >>> for those exceptions running on the "normal" kernel stack and returning >>> to kernelspace: because the topmost frame is ignored by the reliable stack >>> tracer anyway, returns to userspace don't need to take care of clearing >>> the marker. >>> >>> Furthermore, as I don't have the ability to test this on Book 3E or >>> 32 bits, limit the change to Book 3S and 64 bits. >>> >>> Finally, make the HAVE_RELIABLE_STACKTRACE Kconfig option depend on >>> PPC_BOOK3S_64 for documentation purposes. Before this patch, it depended >>> on PPC64 && CPU_LITTLE_ENDIAN and because CPU_LITTLE_ENDIAN implies >>> PPC_BOOK3S_64, there's no functional change here. >> >> That has nothing to do with the fix and should really be in a separate >> patch. >> >> I can split it when applying. > > If you don't mind, that would be nice! Or simply drop that > chunk... Otherwise, let me know if I shall send a split v2 for this > patch [1/4] only. No worries, I split it out: commit a50d3250d7ae34c561177a1f9cfb79816fcbcff1 Author: Nicolai Stange <nstange@suse.de> AuthorDate: Thu Jan 31 16:41:50 2019 +1100 Commit: Michael Ellerman <mpe@ellerman.id.au> CommitDate: Thu Jan 31 16:43:29 2019 +1100 powerpc/64s: Make reliable stacktrace dependency clearer Make the HAVE_RELIABLE_STACKTRACE Kconfig option depend on PPC_BOOK3S_64 for documentation purposes. Before this patch, it depended on PPC64 && CPU_LITTLE_ENDIAN and because CPU_LITTLE_ENDIAN implies PPC_BOOK3S_64, there's no functional change here. Signed-off-by: Nicolai Stange <nstange@suse.de> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> [mpe: Split out of larger patch] Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 2890d36eb531..73bf87b1d274 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -220,7 +220,7 @@ config PPC select HAVE_PERF_USER_STACK_DUMP select HAVE_RCU_TABLE_FREE if SMP select HAVE_REGS_AND_STACK_ACCESS_API - select HAVE_RELIABLE_STACKTRACE if PPC64 && CPU_LITTLE_ENDIAN + select HAVE_RELIABLE_STACKTRACE if PPC_BOOK3S_64 && CPU_LITTLE_ENDIAN select HAVE_SYSCALL_TRACEPOINTS select HAVE_VIRT_CPU_ACCOUNTING select HAVE_IRQ_TIME_ACCOUNTING cheers
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au> To: Nicolai Stange <nstange@suse.de> Cc: Joe Lawrence <joe.lawrence@redhat.com>, Nicolai Stange <nstange@suse.de>, Jiri Kosina <jkosina@suse.cz>, linux-kernel@vger.kernel.org, Torsten Duwe <duwe@lst.de>, Josh Poimboeuf <jpoimboe@redhat.com>, live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 1/4] powerpc/64s: Clear on-stack exception marker upon exception return Date: Thu, 31 Jan 2019 16:46:05 +1100 [thread overview] Message-ID: <87zhrhnzvm.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <87a7jirrn6.fsf@suse.de> Nicolai Stange <nstange@suse.de> writes: > Michael Ellerman <mpe@ellerman.id.au> writes: > >> Joe Lawrence <joe.lawrence@redhat.com> writes: >>> From: Nicolai Stange <nstange@suse.de> >>> >>> The ppc64 specific implementation of the reliable stacktracer, >>> save_stack_trace_tsk_reliable(), bails out and reports an "unreliable >>> trace" whenever it finds an exception frame on the stack. Stack frames >>> are classified as exception frames if the STACK_FRAME_REGS_MARKER magic, >>> as written by exception prologues, is found at a particular location. >>> >>> However, as observed by Joe Lawrence, it is possible in practice that >>> non-exception stack frames can alias with prior exception frames and thus, >>> that the reliable stacktracer can find a stale STACK_FRAME_REGS_MARKER on >>> the stack. It in turn falsely reports an unreliable stacktrace and blocks >>> any live patching transition to finish. Said condition lasts until the >>> stack frame is overwritten/initialized by function call or other means. >>> >>> In principle, we could mitigate this by making the exception frame >>> classification condition in save_stack_trace_tsk_reliable() stronger: >>> in addition to testing for STACK_FRAME_REGS_MARKER, we could also take into >>> account that for all exceptions executing on the kernel stack >>> - their stack frames's backlink pointers always match what is saved >>> in their pt_regs instance's ->gpr[1] slot and that >>> - their exception frame size equals STACK_INT_FRAME_SIZE, a value >>> uncommonly large for non-exception frames. >>> >>> However, while these are currently true, relying on them would make the >>> reliable stacktrace implementation more sensitive towards future changes in >>> the exception entry code. Note that false negatives, i.e. not detecting >>> exception frames, would silently break the live patching consistency model. >>> >>> Furthermore, certain other places (diagnostic stacktraces, perf, xmon) >>> rely on STACK_FRAME_REGS_MARKER as well. >>> >>> Make the exception exit code clear the on-stack STACK_FRAME_REGS_MARKER >>> for those exceptions running on the "normal" kernel stack and returning >>> to kernelspace: because the topmost frame is ignored by the reliable stack >>> tracer anyway, returns to userspace don't need to take care of clearing >>> the marker. >>> >>> Furthermore, as I don't have the ability to test this on Book 3E or >>> 32 bits, limit the change to Book 3S and 64 bits. >>> >>> Finally, make the HAVE_RELIABLE_STACKTRACE Kconfig option depend on >>> PPC_BOOK3S_64 for documentation purposes. Before this patch, it depended >>> on PPC64 && CPU_LITTLE_ENDIAN and because CPU_LITTLE_ENDIAN implies >>> PPC_BOOK3S_64, there's no functional change here. >> >> That has nothing to do with the fix and should really be in a separate >> patch. >> >> I can split it when applying. > > If you don't mind, that would be nice! Or simply drop that > chunk... Otherwise, let me know if I shall send a split v2 for this > patch [1/4] only. No worries, I split it out: commit a50d3250d7ae34c561177a1f9cfb79816fcbcff1 Author: Nicolai Stange <nstange@suse.de> AuthorDate: Thu Jan 31 16:41:50 2019 +1100 Commit: Michael Ellerman <mpe@ellerman.id.au> CommitDate: Thu Jan 31 16:43:29 2019 +1100 powerpc/64s: Make reliable stacktrace dependency clearer Make the HAVE_RELIABLE_STACKTRACE Kconfig option depend on PPC_BOOK3S_64 for documentation purposes. Before this patch, it depended on PPC64 && CPU_LITTLE_ENDIAN and because CPU_LITTLE_ENDIAN implies PPC_BOOK3S_64, there's no functional change here. Signed-off-by: Nicolai Stange <nstange@suse.de> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> [mpe: Split out of larger patch] Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 2890d36eb531..73bf87b1d274 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -220,7 +220,7 @@ config PPC select HAVE_PERF_USER_STACK_DUMP select HAVE_RCU_TABLE_FREE if SMP select HAVE_REGS_AND_STACK_ACCESS_API - select HAVE_RELIABLE_STACKTRACE if PPC64 && CPU_LITTLE_ENDIAN + select HAVE_RELIABLE_STACKTRACE if PPC_BOOK3S_64 && CPU_LITTLE_ENDIAN select HAVE_SYSCALL_TRACEPOINTS select HAVE_VIRT_CPU_ACCOUNTING select HAVE_IRQ_TIME_ACCOUNTING cheers
next prev parent reply other threads:[~2019-01-31 5:46 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-22 15:57 [PATCH 0/4] powerpc/livepatch: reliable stack unwinder fixes Joe Lawrence 2019-01-22 15:57 ` Joe Lawrence 2019-01-22 15:57 ` [PATCH 1/4] powerpc/64s: Clear on-stack exception marker upon exception return Joe Lawrence 2019-01-22 15:57 ` Joe Lawrence 2019-01-30 12:27 ` Michael Ellerman 2019-01-30 12:27 ` Michael Ellerman 2019-01-30 17:18 ` Nicolai Stange 2019-01-30 17:18 ` Nicolai Stange 2019-01-31 5:46 ` Michael Ellerman [this message] 2019-01-31 5:46 ` Michael Ellerman 2019-02-02 1:14 ` Balbir Singh 2019-02-02 1:14 ` Balbir Singh 2019-02-02 3:42 ` Balbir Singh 2019-02-02 3:42 ` Balbir Singh 2019-02-05 11:24 ` Michael Ellerman 2019-02-05 11:24 ` Michael Ellerman 2019-02-06 2:48 ` Balbir Singh 2019-02-06 2:48 ` Balbir Singh 2019-02-06 4:44 ` Michael Ellerman 2019-02-06 4:44 ` Michael Ellerman 2019-02-06 8:45 ` Balbir Singh 2019-02-06 8:45 ` Balbir Singh 2019-02-08 13:02 ` [1/4] " Michael Ellerman 2019-02-08 13:02 ` Michael Ellerman 2019-01-22 15:57 ` [PATCH 2/4] powerpc/livepatch: relax reliable stack tracer checks for first-frame Joe Lawrence 2019-01-22 15:57 ` Joe Lawrence 2019-01-22 15:57 ` [PATCH 3/4] powerpc/livepatch: small cleanups in save_stack_trace_tsk_reliable() Joe Lawrence 2019-01-22 15:57 ` Joe Lawrence 2019-01-22 15:57 ` [PATCH 4/4] powerpc/livepatch: return -ERRNO values " Joe Lawrence 2019-01-22 15:57 ` Joe Lawrence 2019-02-02 0:59 ` Balbir Singh 2019-02-02 0:59 ` Balbir Singh 2019-01-29 21:10 ` [PATCH 0/4] powerpc/livepatch: reliable stack unwinder fixes Josh Poimboeuf 2019-01-29 21:10 ` Josh Poimboeuf 2019-01-29 23:50 ` Jiri Kosina 2019-01-29 23:50 ` Jiri Kosina 2019-01-30 12:22 ` Michael Ellerman 2019-01-30 12:22 ` Michael Ellerman 2019-01-30 13:28 ` Jiri Kosina 2019-01-30 13:28 ` Jiri Kosina 2019-01-31 5:46 ` Michael Ellerman 2019-01-31 5:46 ` Michael Ellerman
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=87zhrhnzvm.fsf@concordia.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=bsingharora@gmail.com \ --cc=duwe@lst.de \ --cc=jkosina@suse.cz \ --cc=joe.lawrence@redhat.com \ --cc=jpoimboe@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=live-patching@vger.kernel.org \ --cc=nstange@suse.de \ /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.