| Message ID | 20211015025847.17694-2-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Reorganize the unwinder and implement stack trace reliability checks | expand |
On Thu, Oct 14, 2021 at 09:58:37PM -0500, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Currently, there are multiple functions in ARM64 code that walk the > stack using start_backtrace() and unwind_frame() or start_backtrace() > and walk_stackframe(). They should all be converted to use > arch_stack_walk(). This makes maintenance easier. Reviwed-by: Mark Brown <broonie@kernel.org> Arguably this should be squashed in with the first user but that's getting bikesheddy and could make hassle merging things in so meh.
On 10/15/21 1:28 PM, Mark Brown wrote: > On Thu, Oct 14, 2021 at 09:58:37PM -0500, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Currently, there are multiple functions in ARM64 code that walk the >> stack using start_backtrace() and unwind_frame() or start_backtrace() >> and walk_stackframe(). They should all be converted to use >> arch_stack_walk(). This makes maintenance easier. > > Reviwed-by: Mark Brown <broonie@kernel.org> > > Arguably this should be squashed in with the first user but that's > getting bikesheddy and could make hassle merging things in so meh. > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Thanks. Madhavan
Hi Madhavan, Apolgoies for the delay in getting round to this. On Thu, Oct 14, 2021 at 09:58:37PM -0500, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Currently, there are multiple functions in ARM64 code that walk the > stack using start_backtrace() and unwind_frame() or start_backtrace() > and walk_stackframe(). They should all be converted to use > arch_stack_walk(). This makes maintenance easier. > > To do that, arch_stack_walk() must always be defined. arch_stack_walk() > is within #ifdef CONFIG_STACKTRACE. So, select STACKTRACE in > arch/arm64/Kconfig. I'd prefer if we could decouple ARCH_STACKWALK from STACKTRACE, so that we don't have to expose /proc/*/stack unconditionally, which Peter Zijlstra has a patch for: https://lore.kernel.org/lkml/20211022152104.356586621@infradead.org/ ... but regardless the rest of the series looks pretty good, so I'll go review that, and we can figure out how to queue the bits and pieces in the right order. Thanks, Mark. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > --- > arch/arm64/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index fdcd54d39c1e..bfb0ce60d820 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -35,6 +35,7 @@ config ARM64 > select ARCH_HAS_SET_DIRECT_MAP > select ARCH_HAS_SET_MEMORY > select ARCH_STACKWALK > + select STACKTRACE > select ARCH_HAS_STRICT_KERNEL_RWX > select ARCH_HAS_STRICT_MODULE_RWX > select ARCH_HAS_SYNC_DMA_FOR_DEVICE > -- > 2.25.1 >
On Fri, Oct 22, 2021 at 07:02:43PM +0100, Mark Rutland wrote: > On Thu, Oct 14, 2021 at 09:58:37PM -0500, madvenka@linux.microsoft.com wrote: > > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > > > Currently, there are multiple functions in ARM64 code that walk the > > stack using start_backtrace() and unwind_frame() or start_backtrace() > > and walk_stackframe(). They should all be converted to use > > arch_stack_walk(). This makes maintenance easier. > > > > To do that, arch_stack_walk() must always be defined. arch_stack_walk() > > is within #ifdef CONFIG_STACKTRACE. So, select STACKTRACE in > > arch/arm64/Kconfig. > > I'd prefer if we could decouple ARCH_STACKWALK from STACKTRACE, so that > we don't have to expose /proc/*/stack unconditionally, which Peter > Zijlstra has a patch for: > > https://lore.kernel.org/lkml/20211022152104.356586621@infradead.org/ > > ... but regardless the rest of the series looks pretty good, so I'll go > review that, and we can figure out how to queue the bits and pieces in > the right order. FWIW, it looks like the direction of travel there is not go and unify the various arch unwinders, but I would like to not depend on STACKTRACE. Regardless, the initial arch_stack_walk() cleanup patches all look good, so I reckon we should try to get those out of the way and queue those for arm64 soon even if we need some more back-and-forth over the later part of the series. With that in mind, I've picked up Peter's patch decoupling ARCH_STACKWALK from STACKTRACE, and rebased the initial patches from this series atop. Since there's some subtltety in a few cases (and this was easy to miss while reviewing), I've expanded the commit messages with additional rationale as to why each transformation is safe. I've pushed that to: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/arch-stack-walk There's a dependency on: https://lore.kernel.org/r/20211029162245.39761-1-mark.rutland@arm.com ... which was queued for v5.16-rc1, but got dropped due to a conflict, and I'm expecting it to be re-queued as a fix for v5.16-rc2 shortly after v5.16-rc1 is tagged. Hopefully that means we have a table base by v5.16-rc2. I'll send the preparatory series as I've prepared it shortly after v5.16-rc1 so that people can shout if I've messed something up. Hopefully it's easy enough to use that as a base for the more involved rework later in this series. Thanks, Mark. > Thanks, > Mark. > > > > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > > --- > > arch/arm64/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index fdcd54d39c1e..bfb0ce60d820 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -35,6 +35,7 @@ config ARM64 > > select ARCH_HAS_SET_DIRECT_MAP > > select ARCH_HAS_SET_MEMORY > > select ARCH_STACKWALK > > + select STACKTRACE > > select ARCH_HAS_STRICT_KERNEL_RWX > > select ARCH_HAS_STRICT_MODULE_RWX > > select ARCH_HAS_SYNC_DMA_FOR_DEVICE > > -- > > 2.25.1 > >
I reviewed the changes briefly. They look good. I will take a more detailed look this week. Thanks for doing this! Once this is part of v5.16-rc2, I will send out version 11 on top of that with the rest of the patches in my series. Madhavan On 11/12/21 11:44 AM, Mark Rutland wrote: > On Fri, Oct 22, 2021 at 07:02:43PM +0100, Mark Rutland wrote: >> On Thu, Oct 14, 2021 at 09:58:37PM -0500, madvenka@linux.microsoft.com wrote: >>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >>> >>> Currently, there are multiple functions in ARM64 code that walk the >>> stack using start_backtrace() and unwind_frame() or start_backtrace() >>> and walk_stackframe(). They should all be converted to use >>> arch_stack_walk(). This makes maintenance easier. >>> >>> To do that, arch_stack_walk() must always be defined. arch_stack_walk() >>> is within #ifdef CONFIG_STACKTRACE. So, select STACKTRACE in >>> arch/arm64/Kconfig. >> >> I'd prefer if we could decouple ARCH_STACKWALK from STACKTRACE, so that >> we don't have to expose /proc/*/stack unconditionally, which Peter >> Zijlstra has a patch for: >> >> https://lore.kernel.org/lkml/20211022152104.356586621@infradead.org/ >> >> ... but regardless the rest of the series looks pretty good, so I'll go >> review that, and we can figure out how to queue the bits and pieces in >> the right order. > > FWIW, it looks like the direction of travel there is not go and unify > the various arch unwinders, but I would like to not depend on > STACKTRACE. Regardless, the initial arch_stack_walk() cleanup patches > all look good, so I reckon we should try to get those out of the way and > queue those for arm64 soon even if we need some more back-and-forth over > the later part of the series. > > With that in mind, I've picked up Peter's patch decoupling > ARCH_STACKWALK from STACKTRACE, and rebased the initial patches from > this series atop. Since there's some subtltety in a few cases (and this > was easy to miss while reviewing), I've expanded the commit messages > with additional rationale as to why each transformation is safe. > I've pushed that to: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/arch-stack-walk > > There's a dependency on: > > https://lore.kernel.org/r/20211029162245.39761-1-mark.rutland@arm.com > > ... which was queued for v5.16-rc1, but got dropped due to a conflict, > and I'm expecting it to be re-queued as a fix for v5.16-rc2 shortly > after v5.16-rc1 is tagged. Hopefully that means we have a table base by > v5.16-rc2. > > I'll send the preparatory series as I've prepared it shortly after > v5.16-rc1 so that people can shout if I've messed something up. > > Hopefully it's easy enough to use that as a base for the more involved > rework later in this series. > > Thanks, > Mark. > >> Thanks, >> Mark. >> >>> >>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >>> --- >>> arch/arm64/Kconfig | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >>> index fdcd54d39c1e..bfb0ce60d820 100644 >>> --- a/arch/arm64/Kconfig >>> +++ b/arch/arm64/Kconfig >>> @@ -35,6 +35,7 @@ config ARM64 >>> select ARCH_HAS_SET_DIRECT_MAP >>> select ARCH_HAS_SET_MEMORY >>> select ARCH_STACKWALK >>> + select STACKTRACE >>> select ARCH_HAS_STRICT_KERNEL_RWX >>> select ARCH_HAS_STRICT_MODULE_RWX >>> select ARCH_HAS_SYNC_DMA_FOR_DEVICE >>> -- >>> 2.25.1 >>>
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index fdcd54d39c1e..bfb0ce60d820 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -35,6 +35,7 @@ config ARM64 select ARCH_HAS_SET_DIRECT_MAP select ARCH_HAS_SET_MEMORY select ARCH_STACKWALK + select STACKTRACE select ARCH_HAS_STRICT_KERNEL_RWX select ARCH_HAS_STRICT_MODULE_RWX select ARCH_HAS_SYNC_DMA_FOR_DEVICE