* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-09 21:26 ` Yang Shi 0 siblings, 0 replies; 14+ messages in thread From: Yang Shi @ 2016-02-09 21:26 UTC (permalink / raw) To: will.deacon, catalin.marinas Cc: linux-kernel, linux-arm-kernel, linaro-kernel, yang.shi dump_backtrace may be called in kthread context, which is not bound to a single cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug report: BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/71 caller is debug_smp_processor_id+0x1c/0x28 CPU: 3 PID: 71 Comm: khungtaskd Not tainted 4.5.0-rc1 #144 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: [<ffffffc00008d120>] dump_backtrace+0x0/0x290 [<ffffffc00008d3d4>] show_stack+0x24/0x30 [<ffffffc0006bfd1c>] dump_stack+0x8c/0xd8 [<ffffffc0006fe56c>] check_preemption_disabled+0x184/0x188 [<ffffffc0006fe58c>] debug_smp_processor_id+0x1c/0x28 [<ffffffc00008d1a4>] dump_backtrace+0x84/0x290 [<ffffffc00008d3d4>] show_stack+0x24/0x30 [<ffffffc00012aa5c>] sched_show_task+0x16c/0x280 [<ffffffc0001f3df8>] watchdog+0x560/0x708 [<ffffffc000111bb8>] kthread+0x1b0/0x1d0 [<ffffffc0000864d0>] ret_from_fork+0x10/0x40 Replace it to raw version to prevent from the race condition. Signed-off-by: Yang Shi <yang.shi@linaro.org> --- arch/arm64/kernel/traps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index cbedd72..9abe236 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -146,7 +146,7 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) { struct stackframe frame; - unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); + unsigned long irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id()); int skip; pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); -- 2.0.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-09 21:26 ` Yang Shi 0 siblings, 0 replies; 14+ messages in thread From: Yang Shi @ 2016-02-09 21:26 UTC (permalink / raw) To: linux-arm-kernel dump_backtrace may be called in kthread context, which is not bound to a single cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug report: BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/71 caller is debug_smp_processor_id+0x1c/0x28 CPU: 3 PID: 71 Comm: khungtaskd Not tainted 4.5.0-rc1 #144 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: [<ffffffc00008d120>] dump_backtrace+0x0/0x290 [<ffffffc00008d3d4>] show_stack+0x24/0x30 [<ffffffc0006bfd1c>] dump_stack+0x8c/0xd8 [<ffffffc0006fe56c>] check_preemption_disabled+0x184/0x188 [<ffffffc0006fe58c>] debug_smp_processor_id+0x1c/0x28 [<ffffffc00008d1a4>] dump_backtrace+0x84/0x290 [<ffffffc00008d3d4>] show_stack+0x24/0x30 [<ffffffc00012aa5c>] sched_show_task+0x16c/0x280 [<ffffffc0001f3df8>] watchdog+0x560/0x708 [<ffffffc000111bb8>] kthread+0x1b0/0x1d0 [<ffffffc0000864d0>] ret_from_fork+0x10/0x40 Replace it to raw version to prevent from the race condition. Signed-off-by: Yang Shi <yang.shi@linaro.org> --- arch/arm64/kernel/traps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index cbedd72..9abe236 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -146,7 +146,7 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) { struct stackframe frame; - unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); + unsigned long irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id()); int skip; pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); -- 2.0.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-09 21:26 ` Yang Shi @ 2016-02-10 10:29 ` Will Deacon -1 siblings, 0 replies; 14+ messages in thread From: Will Deacon @ 2016-02-10 10:29 UTC (permalink / raw) To: Yang Shi Cc: catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel, james.morse On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: > dump_backtrace may be called in kthread context, which is not bound to a single > cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug > report: If we're preemptible here, it means that our irq_stack_ptr is potentially bogus. Whilst this isn't an issue for kthreads, it does feel like we could make this slightly more robust in the face of potential frame corruption. Maybe just zero the IRQ stack pointer if we're in preemptible context? Will > BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/71 > caller is debug_smp_processor_id+0x1c/0x28 > CPU: 3 PID: 71 Comm: khungtaskd Not tainted 4.5.0-rc1 #144 > Hardware name: Freescale Layerscape 2085a RDB Board (DT) > Call trace: > [<ffffffc00008d120>] dump_backtrace+0x0/0x290 > [<ffffffc00008d3d4>] show_stack+0x24/0x30 > [<ffffffc0006bfd1c>] dump_stack+0x8c/0xd8 > [<ffffffc0006fe56c>] check_preemption_disabled+0x184/0x188 > [<ffffffc0006fe58c>] debug_smp_processor_id+0x1c/0x28 > [<ffffffc00008d1a4>] dump_backtrace+0x84/0x290 > [<ffffffc00008d3d4>] show_stack+0x24/0x30 > [<ffffffc00012aa5c>] sched_show_task+0x16c/0x280 > [<ffffffc0001f3df8>] watchdog+0x560/0x708 > [<ffffffc000111bb8>] kthread+0x1b0/0x1d0 > [<ffffffc0000864d0>] ret_from_fork+0x10/0x40 > > Replace it to raw version to prevent from the race condition. > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > arch/arm64/kernel/traps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index cbedd72..9abe236 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -146,7 +146,7 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > { > struct stackframe frame; > - unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > + unsigned long irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id()); > int skip; > > pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > -- > 2.0.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-10 10:29 ` Will Deacon 0 siblings, 0 replies; 14+ messages in thread From: Will Deacon @ 2016-02-10 10:29 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: > dump_backtrace may be called in kthread context, which is not bound to a single > cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug > report: If we're preemptible here, it means that our irq_stack_ptr is potentially bogus. Whilst this isn't an issue for kthreads, it does feel like we could make this slightly more robust in the face of potential frame corruption. Maybe just zero the IRQ stack pointer if we're in preemptible context? Will > BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/71 > caller is debug_smp_processor_id+0x1c/0x28 > CPU: 3 PID: 71 Comm: khungtaskd Not tainted 4.5.0-rc1 #144 > Hardware name: Freescale Layerscape 2085a RDB Board (DT) > Call trace: > [<ffffffc00008d120>] dump_backtrace+0x0/0x290 > [<ffffffc00008d3d4>] show_stack+0x24/0x30 > [<ffffffc0006bfd1c>] dump_stack+0x8c/0xd8 > [<ffffffc0006fe56c>] check_preemption_disabled+0x184/0x188 > [<ffffffc0006fe58c>] debug_smp_processor_id+0x1c/0x28 > [<ffffffc00008d1a4>] dump_backtrace+0x84/0x290 > [<ffffffc00008d3d4>] show_stack+0x24/0x30 > [<ffffffc00012aa5c>] sched_show_task+0x16c/0x280 > [<ffffffc0001f3df8>] watchdog+0x560/0x708 > [<ffffffc000111bb8>] kthread+0x1b0/0x1d0 > [<ffffffc0000864d0>] ret_from_fork+0x10/0x40 > > Replace it to raw version to prevent from the race condition. > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > arch/arm64/kernel/traps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index cbedd72..9abe236 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -146,7 +146,7 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > { > struct stackframe frame; > - unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > + unsigned long irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id()); > int skip; > > pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > -- > 2.0.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-10 10:29 ` Will Deacon @ 2016-02-10 11:52 ` James Morse -1 siblings, 0 replies; 14+ messages in thread From: James Morse @ 2016-02-10 11:52 UTC (permalink / raw) To: Will Deacon, Yang Shi Cc: catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel On 10/02/16 10:29, Will Deacon wrote: > On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >> dump_backtrace may be called in kthread context, which is not bound to a single >> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >> report: > > If we're preemptible here, it means that our irq_stack_ptr is potentially > bogus. Whilst this isn't an issue for kthreads, it does feel like we > could make this slightly more robust in the face of potential frame > corruption. Maybe just zero the IRQ stack pointer if we're in preemptible > context? Switching between stacks is only valid if we are tracing ourselves while on the irq_stack, we should probably prevent it for other tasks too. Something like (untested): --------------------- if (tsk == current && in_atomic()) irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); else irq_stack_ptr = 0; --------------------- This would work when we trace ourselves while on the irq_stack, but break* tracing a running task on a remote cpu (khungtaskd doesn't do this). The same fix would apply to unwind_frame(), we have 'tsk' in both functions. Thoughts? James * If this were to ever happen, we would fail to switch to the original stack if tracing a remote irq stack, and print the wrong exception args. I'm not aware of any users of this, (panic() sends an IPI). ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-10 11:52 ` James Morse 0 siblings, 0 replies; 14+ messages in thread From: James Morse @ 2016-02-10 11:52 UTC (permalink / raw) To: linux-arm-kernel On 10/02/16 10:29, Will Deacon wrote: > On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >> dump_backtrace may be called in kthread context, which is not bound to a single >> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >> report: > > If we're preemptible here, it means that our irq_stack_ptr is potentially > bogus. Whilst this isn't an issue for kthreads, it does feel like we > could make this slightly more robust in the face of potential frame > corruption. Maybe just zero the IRQ stack pointer if we're in preemptible > context? Switching between stacks is only valid if we are tracing ourselves while on the irq_stack, we should probably prevent it for other tasks too. Something like (untested): --------------------- if (tsk == current && in_atomic()) irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); else irq_stack_ptr = 0; --------------------- This would work when we trace ourselves while on the irq_stack, but break* tracing a running task on a remote cpu (khungtaskd doesn't do this). The same fix would apply to unwind_frame(), we have 'tsk' in both functions. Thoughts? James * If this were to ever happen, we would fail to switch to the original stack if tracing a remote irq stack, and print the wrong exception args. I'm not aware of any users of this, (panic() sends an IPI). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-10 11:52 ` James Morse @ 2016-02-10 12:10 ` Will Deacon -1 siblings, 0 replies; 14+ messages in thread From: Will Deacon @ 2016-02-10 12:10 UTC (permalink / raw) To: James Morse Cc: Yang Shi, catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: > On 10/02/16 10:29, Will Deacon wrote: > > On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: > >> dump_backtrace may be called in kthread context, which is not bound to a single > >> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug > >> report: > > > > If we're preemptible here, it means that our irq_stack_ptr is potentially > > bogus. Whilst this isn't an issue for kthreads, it does feel like we > > could make this slightly more robust in the face of potential frame > > corruption. Maybe just zero the IRQ stack pointer if we're in preemptible > > context? > > Switching between stacks is only valid if we are tracing ourselves while on the > irq_stack, we should probably prevent it for other tasks too. > > Something like (untested): > --------------------- > if (tsk == current && in_atomic()) > irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > else > irq_stack_ptr = 0; > --------------------- > > This would work when we trace ourselves while on the irq_stack, but break* > tracing a running task on a remote cpu (khungtaskd doesn't do this). > > The same fix would apply to unwind_frame(), we have 'tsk' in both functions. > > Thoughts? in_atomic is a misnomer: https://lwn.net/Articles/274695/ ;) So we might be better off zeroing the pointer if tsk != current || preemptible(). But yeah, I think we're in general agreement about this. Will ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-10 12:10 ` Will Deacon 0 siblings, 0 replies; 14+ messages in thread From: Will Deacon @ 2016-02-10 12:10 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: > On 10/02/16 10:29, Will Deacon wrote: > > On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: > >> dump_backtrace may be called in kthread context, which is not bound to a single > >> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug > >> report: > > > > If we're preemptible here, it means that our irq_stack_ptr is potentially > > bogus. Whilst this isn't an issue for kthreads, it does feel like we > > could make this slightly more robust in the face of potential frame > > corruption. Maybe just zero the IRQ stack pointer if we're in preemptible > > context? > > Switching between stacks is only valid if we are tracing ourselves while on the > irq_stack, we should probably prevent it for other tasks too. > > Something like (untested): > --------------------- > if (tsk == current && in_atomic()) > irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > else > irq_stack_ptr = 0; > --------------------- > > This would work when we trace ourselves while on the irq_stack, but break* > tracing a running task on a remote cpu (khungtaskd doesn't do this). > > The same fix would apply to unwind_frame(), we have 'tsk' in both functions. > > Thoughts? in_atomic is a misnomer: https://lwn.net/Articles/274695/ ;) So we might be better off zeroing the pointer if tsk != current || preemptible(). But yeah, I think we're in general agreement about this. Will ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-10 12:10 ` Will Deacon @ 2016-02-10 18:12 ` Shi, Yang -1 siblings, 0 replies; 14+ messages in thread From: Shi, Yang @ 2016-02-10 18:12 UTC (permalink / raw) To: Will Deacon, James Morse Cc: catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel On 2/10/2016 4:10 AM, Will Deacon wrote: > On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >> On 10/02/16 10:29, Will Deacon wrote: >>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>> dump_backtrace may be called in kthread context, which is not bound to a single >>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>> report: >>> >>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>> could make this slightly more robust in the face of potential frame >>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>> context? >> >> Switching between stacks is only valid if we are tracing ourselves while on the >> irq_stack, we should probably prevent it for other tasks too. >> >> Something like (untested): >> --------------------- >> if (tsk == current && in_atomic()) >> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); One follow up question, is it possible to have both tsk != current and on_irq_stack is true at the same time? If it is possible, this may be a problem in unwind_frame called by profile_pc which has tsk being NULL. Thanks, Yang >> else >> irq_stack_ptr = 0; >> --------------------- >> >> This would work when we trace ourselves while on the irq_stack, but break* >> tracing a running task on a remote cpu (khungtaskd doesn't do this). >> >> The same fix would apply to unwind_frame(), we have 'tsk' in both functions. >> >> Thoughts? > > in_atomic is a misnomer: > > https://lwn.net/Articles/274695/ > > ;) > > So we might be better off zeroing the pointer if tsk != current || > preemptible(). But yeah, I think we're in general agreement about this. > > Will > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-10 18:12 ` Shi, Yang 0 siblings, 0 replies; 14+ messages in thread From: Shi, Yang @ 2016-02-10 18:12 UTC (permalink / raw) To: linux-arm-kernel On 2/10/2016 4:10 AM, Will Deacon wrote: > On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >> On 10/02/16 10:29, Will Deacon wrote: >>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>> dump_backtrace may be called in kthread context, which is not bound to a single >>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>> report: >>> >>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>> could make this slightly more robust in the face of potential frame >>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>> context? >> >> Switching between stacks is only valid if we are tracing ourselves while on the >> irq_stack, we should probably prevent it for other tasks too. >> >> Something like (untested): >> --------------------- >> if (tsk == current && in_atomic()) >> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); One follow up question, is it possible to have both tsk != current and on_irq_stack is true at the same time? If it is possible, this may be a problem in unwind_frame called by profile_pc which has tsk being NULL. Thanks, Yang >> else >> irq_stack_ptr = 0; >> --------------------- >> >> This would work when we trace ourselves while on the irq_stack, but break* >> tracing a running task on a remote cpu (khungtaskd doesn't do this). >> >> The same fix would apply to unwind_frame(), we have 'tsk' in both functions. >> >> Thoughts? > > in_atomic is a misnomer: > > https://lwn.net/Articles/274695/ > > ;) > > So we might be better off zeroing the pointer if tsk != current || > preemptible(). But yeah, I think we're in general agreement about this. > > Will > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-10 18:12 ` Shi, Yang @ 2016-02-11 10:41 ` James Morse -1 siblings, 0 replies; 14+ messages in thread From: James Morse @ 2016-02-11 10:41 UTC (permalink / raw) To: Shi, Yang Cc: Will Deacon, catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel Hi! On 10/02/16 18:12, Shi, Yang wrote: > On 2/10/2016 4:10 AM, Will Deacon wrote: >> On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >>> On 10/02/16 10:29, Will Deacon wrote: >>>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>>> dump_backtrace may be called in kthread context, which is not bound to a >>>>> single >>>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>>> report: >>>> >>>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>>> could make this slightly more robust in the face of potential frame >>>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>>> context? >>> >>> Switching between stacks is only valid if we are tracing ourselves while on the >>> irq_stack, we should probably prevent it for other tasks too. >>> >>> Something like (untested): >>> --------------------- >>> if (tsk == current && in_atomic()) >>> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > > One follow up question, is it possible to have both tsk != current and > on_irq_stack is true at the same time? No. If you are tracing an irq stack, it must be your own stack. If this weren't the case, it would be the stack of a running task on a remote CPU, and you would be racing with the remote CPU changing the values you are reading. Fortunately nothing tries to do this. (The third case would be tracing a sleeping irq stack - this doesn't happen either, as we switch back to the original stack before calling schedule()). > If it is possible, this may be a problem > in unwind_frame called by profile_pc which has tsk being NULL. Ah, well spotted. I guess there should also be a != NULL comparison thrown into the mix. I don't think it will be a problem for profile_pc() as it should always find a !in_lock_functions() frame before it needs to switch stack, (which we are preventing it from doing). If this ever did happen, it will return 0. Thanks, James ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-11 10:41 ` James Morse 0 siblings, 0 replies; 14+ messages in thread From: James Morse @ 2016-02-11 10:41 UTC (permalink / raw) To: linux-arm-kernel Hi! On 10/02/16 18:12, Shi, Yang wrote: > On 2/10/2016 4:10 AM, Will Deacon wrote: >> On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >>> On 10/02/16 10:29, Will Deacon wrote: >>>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>>> dump_backtrace may be called in kthread context, which is not bound to a >>>>> single >>>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>>> report: >>>> >>>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>>> could make this slightly more robust in the face of potential frame >>>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>>> context? >>> >>> Switching between stacks is only valid if we are tracing ourselves while on the >>> irq_stack, we should probably prevent it for other tasks too. >>> >>> Something like (untested): >>> --------------------- >>> if (tsk == current && in_atomic()) >>> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); > > One follow up question, is it possible to have both tsk != current and > on_irq_stack is true at the same time? No. If you are tracing an irq stack, it must be your own stack. If this weren't the case, it would be the stack of a running task on a remote CPU, and you would be racing with the remote CPU changing the values you are reading. Fortunately nothing tries to do this. (The third case would be tracing a sleeping irq stack - this doesn't happen either, as we switch back to the original stack before calling schedule()). > If it is possible, this may be a problem > in unwind_frame called by profile_pc which has tsk being NULL. Ah, well spotted. I guess there should also be a != NULL comparison thrown into the mix. I don't think it will be a problem for profile_pc() as it should always find a !in_lock_functions() frame before it needs to switch stack, (which we are preventing it from doing). If this ever did happen, it will return 0. Thanks, James ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump 2016-02-11 10:41 ` James Morse @ 2016-02-11 17:36 ` Shi, Yang -1 siblings, 0 replies; 14+ messages in thread From: Shi, Yang @ 2016-02-11 17:36 UTC (permalink / raw) To: James Morse Cc: Will Deacon, catalin.marinas, linux-kernel, linux-arm-kernel, linaro-kernel On 2/11/2016 2:41 AM, James Morse wrote: > Hi! > > On 10/02/16 18:12, Shi, Yang wrote: >> On 2/10/2016 4:10 AM, Will Deacon wrote: >>> On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >>>> On 10/02/16 10:29, Will Deacon wrote: >>>>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>>>> dump_backtrace may be called in kthread context, which is not bound to a >>>>>> single >>>>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>>>> report: >>>>> >>>>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>>>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>>>> could make this slightly more robust in the face of potential frame >>>>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>>>> context? >>>> >>>> Switching between stacks is only valid if we are tracing ourselves while on the >>>> irq_stack, we should probably prevent it for other tasks too. >>>> >>>> Something like (untested): >>>> --------------------- >>>> if (tsk == current && in_atomic()) >>>> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); >> >> One follow up question, is it possible to have both tsk != current and >> on_irq_stack is true at the same time? > > No. If you are tracing an irq stack, it must be your own stack. > > If this weren't the case, it would be the stack of a running task on a remote > CPU, and you would be racing with the remote CPU changing the values you are > reading. Fortunately nothing tries to do this. > > (The third case would be tracing a sleeping irq stack - this doesn't happen > either, as we switch back to the original stack before calling schedule()). > > >> If it is possible, this may be a problem >> in unwind_frame called by profile_pc which has tsk being NULL. > > Ah, well spotted. I guess there should also be a != NULL comparison thrown into > the mix. I don't think it will be a problem for profile_pc() as it should always > find a !in_lock_functions() frame before it needs to switch stack, (which we are > preventing it from doing). If this ever did happen, it will return 0. Thanks for the elaboration. I changed the logic a little bit to: if (tsk == current && !preemptible()) irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); else irq_stack_ptr = 0; In this way, the NULL pointer will be covered by "else" too. v2 patch will be sent out soon once I'm done some smoke testing. Yang > > > Thanks, > > James > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump @ 2016-02-11 17:36 ` Shi, Yang 0 siblings, 0 replies; 14+ messages in thread From: Shi, Yang @ 2016-02-11 17:36 UTC (permalink / raw) To: linux-arm-kernel On 2/11/2016 2:41 AM, James Morse wrote: > Hi! > > On 10/02/16 18:12, Shi, Yang wrote: >> On 2/10/2016 4:10 AM, Will Deacon wrote: >>> On Wed, Feb 10, 2016 at 11:52:31AM +0000, James Morse wrote: >>>> On 10/02/16 10:29, Will Deacon wrote: >>>>> On Tue, Feb 09, 2016 at 01:26:22PM -0800, Yang Shi wrote: >>>>>> dump_backtrace may be called in kthread context, which is not bound to a >>>>>> single >>>>>> cpu, i.e. khungtaskd, then calling smp_processor_id may trigger the below bug >>>>>> report: >>>>> >>>>> If we're preemptible here, it means that our irq_stack_ptr is potentially >>>>> bogus. Whilst this isn't an issue for kthreads, it does feel like we >>>>> could make this slightly more robust in the face of potential frame >>>>> corruption. Maybe just zero the IRQ stack pointer if we're in preemptible >>>>> context? >>>> >>>> Switching between stacks is only valid if we are tracing ourselves while on the >>>> irq_stack, we should probably prevent it for other tasks too. >>>> >>>> Something like (untested): >>>> --------------------- >>>> if (tsk == current && in_atomic()) >>>> irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); >> >> One follow up question, is it possible to have both tsk != current and >> on_irq_stack is true at the same time? > > No. If you are tracing an irq stack, it must be your own stack. > > If this weren't the case, it would be the stack of a running task on a remote > CPU, and you would be racing with the remote CPU changing the values you are > reading. Fortunately nothing tries to do this. > > (The third case would be tracing a sleeping irq stack - this doesn't happen > either, as we switch back to the original stack before calling schedule()). > > >> If it is possible, this may be a problem >> in unwind_frame called by profile_pc which has tsk being NULL. > > Ah, well spotted. I guess there should also be a != NULL comparison thrown into > the mix. I don't think it will be a problem for profile_pc() as it should always > find a !in_lock_functions() frame before it needs to switch stack, (which we are > preventing it from doing). If this ever did happen, it will return 0. Thanks for the elaboration. I changed the logic a little bit to: if (tsk == current && !preemptible()) irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id()); else irq_stack_ptr = 0; In this way, the NULL pointer will be covered by "else" too. v2 patch will be sent out soon once I'm done some smoke testing. Yang > > > Thanks, > > James > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-02-11 17:36 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-09 21:26 [PATCH] arm64: use raw_smp_processor_id in stack backtrace dump Yang Shi 2016-02-09 21:26 ` Yang Shi 2016-02-10 10:29 ` Will Deacon 2016-02-10 10:29 ` Will Deacon 2016-02-10 11:52 ` James Morse 2016-02-10 11:52 ` James Morse 2016-02-10 12:10 ` Will Deacon 2016-02-10 12:10 ` Will Deacon 2016-02-10 18:12 ` Shi, Yang 2016-02-10 18:12 ` Shi, Yang 2016-02-11 10:41 ` James Morse 2016-02-11 10:41 ` James Morse 2016-02-11 17:36 ` Shi, Yang 2016-02-11 17:36 ` Shi, Yang
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.