* [PATCH 0/2] Fix page_owner broken on arm64
@ 2021-03-17 14:20 Chen Jun
2021-03-17 14:20 ` [PATCH 1/2] stacktrace: Move struct stacktrace_cookie to stacktrace.h Chen Jun
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Chen Jun @ 2021-03-17 14:20 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel; +Cc: akpm, catalin.marinas, will, rui.xiang
On arm64, cat /sys/kernel/debug/page_owner
All pages return the same stack
stack_trace_save+0x4c/0x78
register_early_stack+0x34/0x70
init_page_owner+0x34/0x230
page_ext_init+0x1bc/0x1dc
The reason is arch_stack_walk save 2 more entries than before.
To fix it, add skip in arch_stack_walk
*** BLURB HERE ***
1. Prepare for 2, move stacktrace_cookie to .h
2. Fix the problem
Chen Jun (2):
stacktrace: Move struct stacktrace_cookie to stacktrace.h
arm64: stacktrace: Add skip when task == current
arch/arm64/kernel/stacktrace.c | 5 +++--
include/linux/stacktrace.h | 7 +++++++
kernel/stacktrace.c | 7 -------
3 files changed, 10 insertions(+), 9 deletions(-)
--
2.9.4
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] stacktrace: Move struct stacktrace_cookie to stacktrace.h 2021-03-17 14:20 [PATCH 0/2] Fix page_owner broken on arm64 Chen Jun @ 2021-03-17 14:20 ` Chen Jun 2021-03-17 14:20 ` [PATCH 2/2] arm64: stacktrace: Add skip when task == current Chen Jun 2021-03-17 22:23 ` [PATCH 0/2] Fix page_owner broken on arm64 Andrew Morton 2 siblings, 0 replies; 11+ messages in thread From: Chen Jun @ 2021-03-17 14:20 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: akpm, catalin.marinas, will, rui.xiang ARM64 need to modify the stacktrace_cookie->skip. Signed-off-by: Chen Jun <chenjun102@huawei.com> --- include/linux/stacktrace.h | 7 +++++++ kernel/stacktrace.c | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h index 50e2df3..238b276 100644 --- a/include/linux/stacktrace.h +++ b/include/linux/stacktrace.h @@ -25,6 +25,13 @@ unsigned int stack_trace_save_user(unsigned long *store, unsigned int size); /* Internal interfaces. Do not use in generic code */ #ifdef CONFIG_ARCH_STACKWALK +struct stacktrace_cookie { + unsigned long *store; + unsigned int size; + unsigned int skip; + unsigned int len; +}; + /** * stack_trace_consume_fn - Callback for arch_stack_walk() * @cookie: Caller supplied pointer handed back by arch_stack_walk() diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c index 9f8117c..b072e8f 100644 --- a/kernel/stacktrace.c +++ b/kernel/stacktrace.c @@ -71,13 +71,6 @@ EXPORT_SYMBOL_GPL(stack_trace_snprint); #ifdef CONFIG_ARCH_STACKWALK -struct stacktrace_cookie { - unsigned long *store; - unsigned int size; - unsigned int skip; - unsigned int len; -}; - static bool stack_trace_consume_entry(void *cookie, unsigned long addr) { struct stacktrace_cookie *c = cookie; -- 2.9.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-17 14:20 [PATCH 0/2] Fix page_owner broken on arm64 Chen Jun 2021-03-17 14:20 ` [PATCH 1/2] stacktrace: Move struct stacktrace_cookie to stacktrace.h Chen Jun @ 2021-03-17 14:20 ` Chen Jun 2021-03-17 18:36 ` Catalin Marinas 2021-03-17 22:23 ` [PATCH 0/2] Fix page_owner broken on arm64 Andrew Morton 2 siblings, 1 reply; 11+ messages in thread From: Chen Jun @ 2021-03-17 14:20 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: akpm, catalin.marinas, will, rui.xiang On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same stack: stack_trace_save+0x4c/0x78 register_early_stack+0x34/0x70 init_page_owner+0x34/0x230 page_ext_init+0x1bc/0x1dc The reason is that: check_recursive_alloc always return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). The root cause is that: commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") make the save_trace save 2 more entries. Add skip in arch_stack_walk when task == current. Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") Signed-off-by: Chen Jun <chenjun102@huawei.com> --- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index ad20981..c26b0ac 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, if (regs) start_backtrace(&frame, regs->regs[29], regs->pc); - else if (task == current) + else if (task == current) { + ((struct stacktrace_cookie *)cookie)->skip += 2; start_backtrace(&frame, (unsigned long)__builtin_frame_address(0), (unsigned long)arch_stack_walk); - else + } else start_backtrace(&frame, thread_saved_fp(task), thread_saved_pc(task)); -- 2.9.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-17 14:20 ` [PATCH 2/2] arm64: stacktrace: Add skip when task == current Chen Jun @ 2021-03-17 18:36 ` Catalin Marinas 2021-03-17 19:34 ` Mark Rutland 0 siblings, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2021-03-17 18:36 UTC (permalink / raw) To: Chen Jun Cc: linux-kernel, linux-arm-kernel, akpm, will, rui.xiang, Mark Brown On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > stack: > stack_trace_save+0x4c/0x78 > register_early_stack+0x34/0x70 > init_page_owner+0x34/0x230 > page_ext_init+0x1bc/0x1dc > > The reason is that: > check_recursive_alloc always return 1 because that > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > The root cause is that: > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > make the save_trace save 2 more entries. > > Add skip in arch_stack_walk when task == current. > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > Signed-off-by: Chen Jun <chenjun102@huawei.com> > --- > arch/arm64/kernel/stacktrace.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index ad20981..c26b0ac 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > if (regs) > start_backtrace(&frame, regs->regs[29], regs->pc); > - else if (task == current) > + else if (task == current) { > + ((struct stacktrace_cookie *)cookie)->skip += 2; > start_backtrace(&frame, > (unsigned long)__builtin_frame_address(0), > (unsigned long)arch_stack_walk); > - else > + } else > start_backtrace(&frame, thread_saved_fp(task), > thread_saved_pc(task)); I don't like abusing the cookie here. It's void * as it's meant to be an opaque type. I'd rather skip the first two frames in walk_stackframe() instead before invoking fn(). Prior to the conversion to ARCH_STACKWALK, we were indeed skipping two more entries in __save_stack_trace() if tsk == current. Something like below, completely untested: diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index ad20981dfda4..2a9f759aa41a 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -115,10 +115,15 @@ NOKPROBE_SYMBOL(unwind_frame); void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, bool (*fn)(void *, unsigned long), void *data) { + /* for the current task, we don't want this function nor its caller */ + int skip = tsk == current ? 2 : 0; + while (1) { int ret; - if (!fn(data, frame->pc)) + if (skip) + skip--; + else if (!fn(data, frame->pc)) break; ret = unwind_frame(tsk, frame); if (ret < 0) -- Catalin ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-17 18:36 ` Catalin Marinas @ 2021-03-17 19:34 ` Mark Rutland 2021-03-18 3:24 ` chenjun (AM) 2021-03-18 16:17 ` Catalin Marinas 0 siblings, 2 replies; 11+ messages in thread From: Mark Rutland @ 2021-03-17 19:34 UTC (permalink / raw) To: Catalin Marinas Cc: Chen Jun, linux-kernel, linux-arm-kernel, akpm, will, rui.xiang, Mark Brown On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > stack: > > stack_trace_save+0x4c/0x78 > > register_early_stack+0x34/0x70 > > init_page_owner+0x34/0x230 > > page_ext_init+0x1bc/0x1dc > > > > The reason is that: > > check_recursive_alloc always return 1 because that > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > The root cause is that: > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > make the save_trace save 2 more entries. > > > > Add skip in arch_stack_walk when task == current. > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > Signed-off-by: Chen Jun <chenjun102@huawei.com> > > --- > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > index ad20981..c26b0ac 100644 > > --- a/arch/arm64/kernel/stacktrace.c > > +++ b/arch/arm64/kernel/stacktrace.c > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > if (regs) > > start_backtrace(&frame, regs->regs[29], regs->pc); > > - else if (task == current) > > + else if (task == current) { > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > start_backtrace(&frame, > > (unsigned long)__builtin_frame_address(0), > > (unsigned long)arch_stack_walk); > > - else > > + } else > > start_backtrace(&frame, thread_saved_fp(task), > > thread_saved_pc(task)); > > I don't like abusing the cookie here. It's void * as it's meant to be an > opaque type. I'd rather skip the first two frames in walk_stackframe() > instead before invoking fn(). I agree that we shouldn't touch cookie here. I don't think that it's right to bodge this inside walk_stackframe(), since that'll add bogus skipping for the case starting with regs in the current task. If we need a bodge, it has to live in arch_stack_walk() where we set up the initial unwinding state. In another thread, we came to the conclusion that arch_stack_walk() should start at its parent, and its parent should add any skipping it requires. Currently, arch_stack_walk() is off-by-one, and we can bodge that by using __builtin_frame_address(1), though I'm waiting for some compiler folk to confirm that's sound. Otherwise we need to add an assembly trampoline to snapshot the FP, which is unfortunastely convoluted. This report suggests that a caller of arch_stack_walk() is off-by-one too, which suggests a larger cross-architecture semantic issue. I'll try to take a look tomorrow. Thanks, Mark. > > Prior to the conversion to ARCH_STACKWALK, we were indeed skipping two > more entries in __save_stack_trace() if tsk == current. Something like > below, completely untested: > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index ad20981dfda4..2a9f759aa41a 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -115,10 +115,15 @@ NOKPROBE_SYMBOL(unwind_frame); > void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, > bool (*fn)(void *, unsigned long), void *data) > { > + /* for the current task, we don't want this function nor its caller */ > + int skip = tsk == current ? 2 : 0; > + > while (1) { > int ret; > > - if (!fn(data, frame->pc)) > + if (skip) > + skip--; > + else if (!fn(data, frame->pc)) > break; > ret = unwind_frame(tsk, frame); > if (ret < 0) > > > -- > Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-17 19:34 ` Mark Rutland @ 2021-03-18 3:24 ` chenjun (AM) 2021-03-18 13:22 ` chenjun (AM) 2021-03-18 16:17 ` Catalin Marinas 1 sibling, 1 reply; 11+ messages in thread From: chenjun (AM) @ 2021-03-18 3:24 UTC (permalink / raw) To: Mark Rutland, Catalin Marinas Cc: linux-kernel, linux-arm-kernel, akpm, will, Xiangrui (Euler), Mark Brown, Wangkefeng (OS Kernel Lab) 在 2021/3/18 3:34, Mark Rutland 写道: > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: >> On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: >>> On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same >>> stack: >>> stack_trace_save+0x4c/0x78 >>> register_early_stack+0x34/0x70 >>> init_page_owner+0x34/0x230 >>> page_ext_init+0x1bc/0x1dc >>> >>> The reason is that: >>> check_recursive_alloc always return 1 because that >>> entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). >>> >>> The root cause is that: >>> commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") >>> make the save_trace save 2 more entries. >>> >>> Add skip in arch_stack_walk when task == current. >>> >>> Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") >>> Signed-off-by: Chen Jun <chenjun102@huawei.com> >>> --- >>> arch/arm64/kernel/stacktrace.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index ad20981..c26b0ac 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, >>> >>> if (regs) >>> start_backtrace(&frame, regs->regs[29], regs->pc); >>> - else if (task == current) >>> + else if (task == current) { >>> + ((struct stacktrace_cookie *)cookie)->skip += 2; >>> start_backtrace(&frame, >>> (unsigned long)__builtin_frame_address(0), >>> (unsigned long)arch_stack_walk); >>> - else >>> + } else >>> start_backtrace(&frame, thread_saved_fp(task), >>> thread_saved_pc(task)); >> >> I don't like abusing the cookie here. It's void * as it's meant to be an >> opaque type. I'd rather skip the first two frames in walk_stackframe() >> instead before invoking fn(). > > I agree that we shouldn't touch cookie here. > > I don't think that it's right to bodge this inside walk_stackframe(), > since that'll add bogus skipping for the case starting with regs in the > current task. If we need a bodge, it has to live in arch_stack_walk() > where we set up the initial unwinding state. > > In another thread, we came to the conclusion that arch_stack_walk() > should start at its parent, and its parent should add any skipping it > requires. > > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > using __builtin_frame_address(1), though I'm waiting for some compiler > folk to confirm that's sound. Otherwise we need to add an assembly > trampoline to snapshot the FP, which is unfortunastely convoluted. > > This report suggests that a caller of arch_stack_walk() is off-by-one > too, which suggests a larger cross-architecture semantic issue. I'll try > to take a look tomorrow. > > Thanks, > Mark. > >> >> Prior to the conversion to ARCH_STACKWALK, we were indeed skipping two >> more entries in __save_stack_trace() if tsk == current. Something like >> below, completely untested: >> >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index ad20981dfda4..2a9f759aa41a 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -115,10 +115,15 @@ NOKPROBE_SYMBOL(unwind_frame); >> void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, >> bool (*fn)(void *, unsigned long), void *data) >> { >> + /* for the current task, we don't want this function nor its caller */ >> + int skip = tsk == current ? 2 : 0; >> + >> while (1) { >> int ret; >> >> - if (!fn(data, frame->pc)) >> + if (skip) >> + skip--; >> + else if (!fn(data, frame->pc)) >> break; >> ret = unwind_frame(tsk, frame); >> if (ret < 0) >> >> >> -- >> Catalin > This change will make kmemleak broken. Maybe the reason is what Mark pointed out. I will try to check out. -- Regards Chen Jun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-18 3:24 ` chenjun (AM) @ 2021-03-18 13:22 ` chenjun (AM) 0 siblings, 0 replies; 11+ messages in thread From: chenjun (AM) @ 2021-03-18 13:22 UTC (permalink / raw) To: Mark Rutland, Catalin Marinas Cc: linux-kernel, linux-arm-kernel, akpm, will, Xiangrui (Euler), Mark Brown, Wangkefeng (OS Kernel Lab) 在 2021/3/18 11:31, chenjun (AM) 写道: > 在 2021/3/18 3:34, Mark Rutland 写道: >> On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: >>> On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: >>>> On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same >>>> stack: >>>> stack_trace_save+0x4c/0x78 >>>> register_early_stack+0x34/0x70 >>>> init_page_owner+0x34/0x230 >>>> page_ext_init+0x1bc/0x1dc >>>> >>>> The reason is that: >>>> check_recursive_alloc always return 1 because that >>>> entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). >>>> >>>> The root cause is that: >>>> commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") >>>> make the save_trace save 2 more entries. >>>> >>>> Add skip in arch_stack_walk when task == current. >>>> >>>> Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") >>>> Signed-off-by: Chen Jun <chenjun102@huawei.com> >>>> --- >>>> arch/arm64/kernel/stacktrace.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>>> index ad20981..c26b0ac 100644 >>>> --- a/arch/arm64/kernel/stacktrace.c >>>> +++ b/arch/arm64/kernel/stacktrace.c >>>> @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, >>>> >>>> if (regs) >>>> start_backtrace(&frame, regs->regs[29], regs->pc); >>>> - else if (task == current) >>>> + else if (task == current) { >>>> + ((struct stacktrace_cookie *)cookie)->skip += 2; >>>> start_backtrace(&frame, >>>> (unsigned long)__builtin_frame_address(0), >>>> (unsigned long)arch_stack_walk); >>>> - else >>>> + } else >>>> start_backtrace(&frame, thread_saved_fp(task), >>>> thread_saved_pc(task)); >>> >>> I don't like abusing the cookie here. It's void * as it's meant to be an >>> opaque type. I'd rather skip the first two frames in walk_stackframe() >>> instead before invoking fn(). >> >> I agree that we shouldn't touch cookie here. >> >> I don't think that it's right to bodge this inside walk_stackframe(), >> since that'll add bogus skipping for the case starting with regs in the >> current task. If we need a bodge, it has to live in arch_stack_walk() >> where we set up the initial unwinding state. >> >> In another thread, we came to the conclusion that arch_stack_walk() >> should start at its parent, and its parent should add any skipping it >> requires. >> >> Currently, arch_stack_walk() is off-by-one, and we can bodge that by >> using __builtin_frame_address(1), though I'm waiting for some compiler >> folk to confirm that's sound. Otherwise we need to add an assembly >> trampoline to snapshot the FP, which is unfortunastely convoluted. >> >> This report suggests that a caller of arch_stack_walk() is off-by-one >> too, which suggests a larger cross-architecture semantic issue. I'll try >> to take a look tomorrow. >> >> Thanks, >> Mark. >> >>> >>> Prior to the conversion to ARCH_STACKWALK, we were indeed skipping two >>> more entries in __save_stack_trace() if tsk == current. Something like >>> below, completely untested: >>> >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index ad20981dfda4..2a9f759aa41a 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -115,10 +115,15 @@ NOKPROBE_SYMBOL(unwind_frame); >>> void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, >>> bool (*fn)(void *, unsigned long), void *data) >>> { >>> + /* for the current task, we don't want this function nor its caller */ >>> + int skip = tsk == current ? 2 : 0; >>> + >>> while (1) { >>> int ret; >>> >>> - if (!fn(data, frame->pc)) >>> + if (skip) >>> + skip--; >>> + else if (!fn(data, frame->pc)) >>> break; >>> ret = unwind_frame(tsk, frame); >>> if (ret < 0) >>> >>> >>> -- >>> Catalin >> > > This change will make kmemleak broken. > Maybe the reason is what Mark pointed out. I will try to check out. > I make a mistake. kmemleak seems to work good. I will do more tests. -- Regards Chen Jun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-17 19:34 ` Mark Rutland 2021-03-18 3:24 ` chenjun (AM) @ 2021-03-18 16:17 ` Catalin Marinas 2021-03-18 17:12 ` Mark Rutland 1 sibling, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2021-03-18 16:17 UTC (permalink / raw) To: Mark Rutland Cc: Chen Jun, linux-kernel, linux-arm-kernel, akpm, will, rui.xiang, Mark Brown On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote: > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > > stack: > > > stack_trace_save+0x4c/0x78 > > > register_early_stack+0x34/0x70 > > > init_page_owner+0x34/0x230 > > > page_ext_init+0x1bc/0x1dc > > > > > > The reason is that: > > > check_recursive_alloc always return 1 because that > > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > > > The root cause is that: > > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > make the save_trace save 2 more entries. > > > > > > Add skip in arch_stack_walk when task == current. > > > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > Signed-off-by: Chen Jun <chenjun102@huawei.com> > > > --- > > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > > index ad20981..c26b0ac 100644 > > > --- a/arch/arm64/kernel/stacktrace.c > > > +++ b/arch/arm64/kernel/stacktrace.c > > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > > > if (regs) > > > start_backtrace(&frame, regs->regs[29], regs->pc); > > > - else if (task == current) > > > + else if (task == current) { > > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > > start_backtrace(&frame, > > > (unsigned long)__builtin_frame_address(0), > > > (unsigned long)arch_stack_walk); > > > - else > > > + } else > > > start_backtrace(&frame, thread_saved_fp(task), > > > thread_saved_pc(task)); > > > > I don't like abusing the cookie here. It's void * as it's meant to be an > > opaque type. I'd rather skip the first two frames in walk_stackframe() > > instead before invoking fn(). > > I agree that we shouldn't touch cookie here. > > I don't think that it's right to bodge this inside walk_stackframe(), > since that'll add bogus skipping for the case starting with regs in the > current task. If we need a bodge, it has to live in arch_stack_walk() > where we set up the initial unwinding state. Good point. However, instead of relying on __builtin_frame_address(1), can we add a 'skip' value to struct stackframe via arch_stack_walk() -> start_backtrace() that is consumed by walk_stackframe()? > In another thread, we came to the conclusion that arch_stack_walk() > should start at its parent, and its parent should add any skipping it > requires. This makes sense. > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > using __builtin_frame_address(1), though I'm waiting for some compiler > folk to confirm that's sound. Otherwise we need to add an assembly > trampoline to snapshot the FP, which is unfortunastely convoluted. > > This report suggests that a caller of arch_stack_walk() is off-by-one > too, which suggests a larger cross-architecture semantic issue. I'll try > to take a look tomorrow. I don't think the caller is off by one, at least not by the final skip value. __set_page_owner() wants the trace to start at its caller. The callee save_stack() in the same file adds a skip of 2. save_stack_trace() increments the skip before invoking arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at its parent, i.e. save_stack_trace(). So save_stack_trace() only need to skip 1 and I think that's in line with the original report where the entries[0] is __set_page_owner(). We only need to skip one. Another untested quick hack (we should probably add the skip argument to start_backtrace()): diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h index eb29b1fe8255..0d32d932ac89 100644 --- a/arch/arm64/include/asm/stacktrace.h +++ b/arch/arm64/include/asm/stacktrace.h @@ -56,6 +56,7 @@ struct stackframe { DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES); unsigned long prev_fp; enum stack_type prev_type; + int skip; #ifdef CONFIG_FUNCTION_GRAPH_TRACER int graph; #endif @@ -153,6 +154,7 @@ static inline void start_backtrace(struct stackframe *frame, { frame->fp = fp; frame->pc = pc; + frame->skip = 0; #ifdef CONFIG_FUNCTION_GRAPH_TRACER frame->graph = 0; #endif diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index ad20981dfda4..a89b2ecbf3de 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -118,7 +118,9 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, while (1) { int ret; - if (!fn(data, frame->pc)) + if (frame->skip > 0) + frame->skip--; + else if (!fn(data, frame->pc)) break; ret = unwind_frame(tsk, frame); if (ret < 0) @@ -201,11 +203,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, if (regs) start_backtrace(&frame, regs->regs[29], regs->pc); - else if (task == current) + else if (task == current) { start_backtrace(&frame, (unsigned long)__builtin_frame_address(0), (unsigned long)arch_stack_walk); - else + frame.skip = 1; + } else start_backtrace(&frame, thread_saved_fp(task), thread_saved_pc(task)); -- Catalin ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-18 16:17 ` Catalin Marinas @ 2021-03-18 17:12 ` Mark Rutland 2021-03-18 18:36 ` Catalin Marinas 0 siblings, 1 reply; 11+ messages in thread From: Mark Rutland @ 2021-03-18 17:12 UTC (permalink / raw) To: Catalin Marinas Cc: Chen Jun, linux-kernel, linux-arm-kernel, akpm, will, rui.xiang, Mark Brown On Thu, Mar 18, 2021 at 04:17:24PM +0000, Catalin Marinas wrote: > On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote: > > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > > > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > > > stack: > > > > stack_trace_save+0x4c/0x78 > > > > register_early_stack+0x34/0x70 > > > > init_page_owner+0x34/0x230 > > > > page_ext_init+0x1bc/0x1dc > > > > > > > > The reason is that: > > > > check_recursive_alloc always return 1 because that > > > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > > > > > The root cause is that: > > > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > make the save_trace save 2 more entries. > > > > > > > > Add skip in arch_stack_walk when task == current. > > > > > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > Signed-off-by: Chen Jun <chenjun102@huawei.com> > > > > --- > > > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > > > index ad20981..c26b0ac 100644 > > > > --- a/arch/arm64/kernel/stacktrace.c > > > > +++ b/arch/arm64/kernel/stacktrace.c > > > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > > > > > if (regs) > > > > start_backtrace(&frame, regs->regs[29], regs->pc); > > > > - else if (task == current) > > > > + else if (task == current) { > > > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > > > start_backtrace(&frame, > > > > (unsigned long)__builtin_frame_address(0), > > > > (unsigned long)arch_stack_walk); > > > > - else > > > > + } else > > > > start_backtrace(&frame, thread_saved_fp(task), > > > > thread_saved_pc(task)); > > > > > > I don't like abusing the cookie here. It's void * as it's meant to be an > > > opaque type. I'd rather skip the first two frames in walk_stackframe() > > > instead before invoking fn(). > > > > I agree that we shouldn't touch cookie here. > > > > I don't think that it's right to bodge this inside walk_stackframe(), > > since that'll add bogus skipping for the case starting with regs in the > > current task. If we need a bodge, it has to live in arch_stack_walk() > > where we set up the initial unwinding state. > > Good point. However, instead of relying on __builtin_frame_address(1), > can we add a 'skip' value to struct stackframe via arch_stack_walk() -> > start_backtrace() that is consumed by walk_stackframe()? We could, but I'd strongly prefer to use __builtin_frame_address(1) if we can, as it's much simpler to read and keeps the logic constrained to the starting function. I'd already hacked that up at: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/unwind&id=5811a76c1be1dcea7104a9a771fc2604bc2a90ef ... and I'm fairly confident that this works on arm64. If __builtin_frame_address(1) is truly unreliable, then we could just manually unwind one step within arch_stack_walk() when unwinding current, which I think is cleaner than spreading this within walk_stackframe(). I can clean up the commit message and post that as a real patch, if you like? > > In another thread, we came to the conclusion that arch_stack_walk() > > should start at its parent, and its parent should add any skipping it > > requires. > > This makes sense. > > > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > > using __builtin_frame_address(1), though I'm waiting for some compiler > > folk to confirm that's sound. Otherwise we need to add an assembly > > trampoline to snapshot the FP, which is unfortunastely convoluted. > > > > This report suggests that a caller of arch_stack_walk() is off-by-one > > too, which suggests a larger cross-architecture semantic issue. I'll try > > to take a look tomorrow. > > I don't think the caller is off by one, at least not by the final skip > value. __set_page_owner() wants the trace to start at its caller. The > callee save_stack() in the same file adds a skip of 2. > save_stack_trace() increments the skip before invoking > arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at > its parent, i.e. save_stack_trace(). FWIW, I had only assumed the caller was also off-by-one because the commit message for this patch said the conversion to ARCH_STACKWALK added two entries. Have I misunderstood, or is that incorrect? So if this is only off-by-one, I agree it's the same problem. Thanks, Mark. > So save_stack_trace() only need to skip 1 and I think that's in line > with the original report where the entries[0] is __set_page_owner(). We > only need to skip one. Another untested quick hack (we should probably > add the skip argument to start_backtrace()): > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h > index eb29b1fe8255..0d32d932ac89 100644 > --- a/arch/arm64/include/asm/stacktrace.h > +++ b/arch/arm64/include/asm/stacktrace.h > @@ -56,6 +56,7 @@ struct stackframe { > DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES); > unsigned long prev_fp; > enum stack_type prev_type; > + int skip; > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > int graph; > #endif > @@ -153,6 +154,7 @@ static inline void start_backtrace(struct stackframe *frame, > { > frame->fp = fp; > frame->pc = pc; > + frame->skip = 0; > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > frame->graph = 0; > #endif > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index ad20981dfda4..a89b2ecbf3de 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -118,7 +118,9 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, > while (1) { > int ret; > > - if (!fn(data, frame->pc)) > + if (frame->skip > 0) > + frame->skip--; > + else if (!fn(data, frame->pc)) > break; > ret = unwind_frame(tsk, frame); > if (ret < 0) > @@ -201,11 +203,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > if (regs) > start_backtrace(&frame, regs->regs[29], regs->pc); > - else if (task == current) > + else if (task == current) { > start_backtrace(&frame, > (unsigned long)__builtin_frame_address(0), > (unsigned long)arch_stack_walk); > - else > + frame.skip = 1; > + } else > start_backtrace(&frame, thread_saved_fp(task), > thread_saved_pc(task)); > > > -- > Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current 2021-03-18 17:12 ` Mark Rutland @ 2021-03-18 18:36 ` Catalin Marinas 0 siblings, 0 replies; 11+ messages in thread From: Catalin Marinas @ 2021-03-18 18:36 UTC (permalink / raw) To: Mark Rutland Cc: Chen Jun, linux-kernel, linux-arm-kernel, akpm, will, rui.xiang, Mark Brown On Thu, Mar 18, 2021 at 05:12:07PM +0000, Mark Rutland wrote: > On Thu, Mar 18, 2021 at 04:17:24PM +0000, Catalin Marinas wrote: > > On Wed, Mar 17, 2021 at 07:34:16PM +0000, Mark Rutland wrote: > > > On Wed, Mar 17, 2021 at 06:36:36PM +0000, Catalin Marinas wrote: > > > > On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote: > > > > > On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same > > > > > stack: > > > > > stack_trace_save+0x4c/0x78 > > > > > register_early_stack+0x34/0x70 > > > > > init_page_owner+0x34/0x230 > > > > > page_ext_init+0x1bc/0x1dc > > > > > > > > > > The reason is that: > > > > > check_recursive_alloc always return 1 because that > > > > > entries[0] is always equal to ip (__set_page_owner+0x3c/0x60). > > > > > > > > > > The root cause is that: > > > > > commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > make the save_trace save 2 more entries. > > > > > > > > > > Add skip in arch_stack_walk when task == current. > > > > > > > > > > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK") > > > > > Signed-off-by: Chen Jun <chenjun102@huawei.com> > > > > > --- > > > > > arch/arm64/kernel/stacktrace.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > > > > index ad20981..c26b0ac 100644 > > > > > --- a/arch/arm64/kernel/stacktrace.c > > > > > +++ b/arch/arm64/kernel/stacktrace.c > > > > > @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > > > > > > > > > > if (regs) > > > > > start_backtrace(&frame, regs->regs[29], regs->pc); > > > > > - else if (task == current) > > > > > + else if (task == current) { > > > > > + ((struct stacktrace_cookie *)cookie)->skip += 2; > > > > > start_backtrace(&frame, > > > > > (unsigned long)__builtin_frame_address(0), > > > > > (unsigned long)arch_stack_walk); > > > > > - else > > > > > + } else > > > > > start_backtrace(&frame, thread_saved_fp(task), > > > > > thread_saved_pc(task)); > > > > > > > > I don't like abusing the cookie here. It's void * as it's meant to be an > > > > opaque type. I'd rather skip the first two frames in walk_stackframe() > > > > instead before invoking fn(). > > > > > > I agree that we shouldn't touch cookie here. > > > > > > I don't think that it's right to bodge this inside walk_stackframe(), > > > since that'll add bogus skipping for the case starting with regs in the > > > current task. If we need a bodge, it has to live in arch_stack_walk() > > > where we set up the initial unwinding state. > > > > Good point. However, instead of relying on __builtin_frame_address(1), > > can we add a 'skip' value to struct stackframe via arch_stack_walk() -> > > start_backtrace() that is consumed by walk_stackframe()? > > We could, but I'd strongly prefer to use __builtin_frame_address(1) if > we can, as it's much simpler to read and keeps the logic constrained to > the starting function. I'd already hacked that up at: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/unwind&id=5811a76c1be1dcea7104a9a771fc2604bc2a90ef > > ... and I'm fairly confident that this works on arm64. If it works with both clang and gcc (and various versions), it's cleaner this way. > If __builtin_frame_address(1) is truly unreliable, then we could just > manually unwind one step within arch_stack_walk() when unwinding > current, which I think is cleaner than spreading this within > walk_stackframe(). > > I can clean up the commit message and post that as a real patch, if you > like? Yes, please. Either variant is fine by me, with a preference for __builtin_frame_address(1) (if we know it works). > > > In another thread, we came to the conclusion that arch_stack_walk() > > > should start at its parent, and its parent should add any skipping it > > > requires. > > > > This makes sense. > > > > > Currently, arch_stack_walk() is off-by-one, and we can bodge that by > > > using __builtin_frame_address(1), though I'm waiting for some compiler > > > folk to confirm that's sound. Otherwise we need to add an assembly > > > trampoline to snapshot the FP, which is unfortunastely convoluted. > > > > > > This report suggests that a caller of arch_stack_walk() is off-by-one > > > too, which suggests a larger cross-architecture semantic issue. I'll try > > > to take a look tomorrow. > > > > I don't think the caller is off by one, at least not by the final skip > > value. __set_page_owner() wants the trace to start at its caller. The > > callee save_stack() in the same file adds a skip of 2. > > save_stack_trace() increments the skip before invoking > > arch_stack_walk(). So far, this assumes that arch_stack_walk() starts at > > its parent, i.e. save_stack_trace(). > > FWIW, I had only assumed the caller was also off-by-one because the > commit message for this patch said the conversion to ARCH_STACKWALK > added two entries. Have I misunderstood, or is that incorrect? I think the commit log is incorrect. Prior to the ARCH_STACKWALK conversion, __save_stack_trace() was skipping 2 since it was creating the initial stack_trace_data and called from save_stack_trace(). After the conversion, the start frame is initialised by arch_stack_walk() which doesn't have any other arch-specific caller it needs to skip. -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix page_owner broken on arm64 2021-03-17 14:20 [PATCH 0/2] Fix page_owner broken on arm64 Chen Jun 2021-03-17 14:20 ` [PATCH 1/2] stacktrace: Move struct stacktrace_cookie to stacktrace.h Chen Jun 2021-03-17 14:20 ` [PATCH 2/2] arm64: stacktrace: Add skip when task == current Chen Jun @ 2021-03-17 22:23 ` Andrew Morton 2 siblings, 0 replies; 11+ messages in thread From: Andrew Morton @ 2021-03-17 22:23 UTC (permalink / raw) To: Chen Jun; +Cc: linux-kernel, linux-arm-kernel, catalin.marinas, will, rui.xiang On Wed, 17 Mar 2021 14:20:48 +0000 Chen Jun <chenjun102@huawei.com> wrote: > On arm64, cat /sys/kernel/debug/page_owner > All pages return the same stack > stack_trace_save+0x4c/0x78 > register_early_stack+0x34/0x70 > init_page_owner+0x34/0x230 > page_ext_init+0x1bc/0x1dc > > The reason is arch_stack_walk save 2 more entries than before. > > To fix it, add skip in arch_stack_walk > > *** BLURB HERE *** > > 1. Prepare for 2, move stacktrace_cookie to .h > 2. Fix the problem > 5fc57df2f6fd was September 2020, so I assume we'll be needing cc:stable on these? (I'll also assume that the arm folks will be handling these) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-18 18:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-17 14:20 [PATCH 0/2] Fix page_owner broken on arm64 Chen Jun 2021-03-17 14:20 ` [PATCH 1/2] stacktrace: Move struct stacktrace_cookie to stacktrace.h Chen Jun 2021-03-17 14:20 ` [PATCH 2/2] arm64: stacktrace: Add skip when task == current Chen Jun 2021-03-17 18:36 ` Catalin Marinas 2021-03-17 19:34 ` Mark Rutland 2021-03-18 3:24 ` chenjun (AM) 2021-03-18 13:22 ` chenjun (AM) 2021-03-18 16:17 ` Catalin Marinas 2021-03-18 17:12 ` Mark Rutland 2021-03-18 18:36 ` Catalin Marinas 2021-03-17 22:23 ` [PATCH 0/2] Fix page_owner broken on arm64 Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).