Fix page_owner broken on arm64
cover Fix page_owner broken on arm64
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(-)
Signature · 1 line
2.9.4
PATCH 1/2 stacktrace: Move struct stacktrace_cookie to stacktrace.h
ARM64 need to modify the stacktrace_cookie->skip.
--- include/linux/stacktrace.h | 7 +++++++ kernel/stacktrace.c | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-)
include/linux/stacktrace.h +7 -0
@@ -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()
kernel/stacktrace.c +0 -7
@@ -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;
Signature · 1 line
2.9.4
PATCH 2/2 arm64: stacktrace: Add skip when task == current
In reply to Chen JunOn 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.
--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm64/kernel/stacktrace.c +3 -2
@@ -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));
Signature · 1 line
2.9.4
Catalin Marinas
maintainerRe: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 31 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -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);
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 211
Quoted 10 lines
arch/arm64/kernel/stacktrace.c +3
-2-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:
arch/arm64/kernel/stacktrace.c +6 -1
@@ -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 )
Signature · 1 line
Catalin
Mark Rutland
reviewerRe: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 211
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().
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.
Quoted 30 lines
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:arch/arm64/kernel/stacktrace.c +6
-1@@ -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 )Signature · 2 lines
-- Catalin
Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 99 lines · Mark Rutland
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 211
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().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 inarch_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.Quoted 28 lines
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:arch/arm64/kernel/stacktrace.c +6
-1@@ -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 )
Quoted 3 lines
Quoted 2 lines
Signature · 1 line
Catalin
This change will make kmemleak broken.
Maybe the reason is what Mark pointed out. I will try to check out.
Signature · 2 lines
Regards Chen Jun
Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 100 lines · chenjun (AM)
Quoted 99 lines · Mark Rutland
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 211
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().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 inarch_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.Quoted 28 lines
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:arch/arm64/kernel/stacktrace.c +6
-1@@ -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 )
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Signature · 1 line
CatalinThis 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.
Signature · 2 lines
Regards Chen Jun
Catalin Marinas
maintainerRe: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
In reply to Mark RutlandQuoted 48 lines · Mark Rutland
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 211
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().
Quoted 6 lines
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 inarch_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()?
Quoted 3 lines
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.
Quoted 8 lines
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()):
arch/arm64/include/asm/stacktrace.h +2 -0
@@ -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
arch/arm64/kernel/stacktrace.c +6 -3
@@ -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));
Signature · 1 line
Catalin
Mark Rutland
reviewerRe: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 51 lines · Catalin Marinas
Quoted 50 lines · Mark Rutland
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 213
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().I agree that we shouldn't touch cookie here.
Quoted 8 lines
Quoted 4 lines
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 inarch_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 viaarch_stack_walk()->start_backtrace()that is consumed bywalk_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:
... 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?
Quoted 15 lines
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 6 lines
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 calleesave_stack()in the same file adds a skip of 2.save_stack_trace()increments the skip before invokingarch_stack_walk(). So far, this assumes thatarch_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.
Quoted 58 lines
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 tostart_backtrace()):arch/arm64/include/asm/stacktrace.h +2
-0@@ -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 arch/arm64/kernel/stacktrace.c +6
-3@@ -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));Signature · 2 lines
-- Catalin
Catalin Marinas
maintainerRe: [PATCH 2/2] arm64: stacktrace: Add skip when task == current
Quoted 61 lines · Mark Rutland
Quoted 51 lines · Catalin Marinas
Quoted 50 lines · Mark Rutland
Quoted 30 lines · Catalin Marinas
Quoted 29 lines · Chen Jun
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/0x1dcThe reason is that:
check_recursive_allocalways return 1 because that entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).The root cause is that:
commit5fc57df2f6fd("arm64: stacktrace: Convert toARCH_STACKWALK") make thesave_tracesave 2 more entries.Add skip in
arch_stack_walkwhen task == current.Fixes 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")Signed-off-by Chen Jun <[email protected]>--- arch/arm64/kernel/stacktrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/stacktrace.c +0
-0@@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,Quoted 16 lines
arch/arm64/kernel/stacktrace.c void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, · line 213
Quoted 12 lines
arch/arm64/kernel/stacktrace.c +3
-2if (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 invokingfn().I agree that we shouldn't touch cookie here.
Quoted 8 lines
Quoted 4 lines
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 inarch_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 viaarch_stack_walk()->start_backtrace()that is consumed bywalk_stackframe()?
Quoted 7 lines
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:... 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.
Quoted 7 lines
If
__builtin_frame_address(1)is truly unreliable, then we could just manually unwind one step withinarch_stack_walk()when unwinding current, which I think is cleaner than spreading this withinwalk_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).
Quoted 22 lines
Quoted 15 lines
Quoted 3 lines
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.
Quoted 8 lines
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.Quoted 6 lines
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 calleesave_stack()in the same file adds a skip of 2.save_stack_trace()increments the skip before invokingarch_stack_walk(). So far, this assumes thatarch_stack_walk()starts at its parent, i.e.save_stack_trace().
Quoted 3 lines
FWIW, I had only assumed the caller was also off-by-one because the commit message for this patch said the conversion to
ARCH_STACKWALKadded 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.
Signature · 1 line
Catalin
Quoted 7 lines · On Wed, 17 Mar 2021 14:20:48 +0000 Chen Jun <[email protected]> wrote:
On arm64, cat
/sys/kernel/debug/page_owner
All pages return the same stackstack_trace_save+0x4c/0x78 register_early_stack+0x34/0x70 init_page_owner+0x34/0x230 page_ext_init+0x1bc/0x1dc
Quoted 9 lines
The reason is
arch_stack_walksave 2 more entries than before.To fix it, add skip in
arch_stack_walk*** BLURB HERE ***
1. Prepare for 2, move
stacktrace_cookieto .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)