| Message ID | 20200506164223.2875760-1-daniel.thompson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3fec4aecb311995189217e64d725cfe84a568de3 |
| Headers | show |
| Series | kgdb: Fix spurious true from in_dbg_master() | expand |
Hi, On Wed, May 6, 2020 at 9:42 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > Currently there is a small window where a badly timed migration could > cause in_dbg_master() to spuriously return true. Specifically if we > migrate to a new core after reading the processor id and the previous > core takes a breakpoint then we will evaluate true if we read > kgdb_active before we get the IPI to bring us to halt. > > Fix this by checking irqs_disabled() first. Interrupts are always > disabled when we are executing the kgdb trap so this is an acceptable > prerequisite. This also allows us to replace raw_smp_processor_id() > with smp_processor_id() since the short circuit logic will prevent > warnings from PREEMPT_DEBUG. > > Fixes: dcc7871128e9 ("kgdb: core changes to support kdb") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > include/linux/kgdb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > index b072aeb1fd78..4d6fe87fd38f 100644 > --- a/include/linux/kgdb.h > +++ b/include/linux/kgdb.h > @@ -323,7 +323,7 @@ extern void gdbstub_exit(int status); > extern int kgdb_single_step; > extern atomic_t kgdb_active; > #define in_dbg_master() \ > - (raw_smp_processor_id() == atomic_read(&kgdb_active)) > + (irqs_disabled() && (smp_processor_id() == atomic_read(&kgdb_active))) > extern bool dbg_is_early; > extern void __init dbg_late_init(void); > extern void kgdb_panic(const char *msg); Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Wed, May 06, 2020 at 05:42:23PM +0100, Daniel Thompson wrote: > Currently there is a small window where a badly timed migration could > cause in_dbg_master() to spuriously return true. Specifically if we > migrate to a new core after reading the processor id and the previous > core takes a breakpoint then we will evaluate true if we read > kgdb_active before we get the IPI to bring us to halt. > > Fix this by checking irqs_disabled() first. Interrupts are always > disabled when we are executing the kgdb trap so this is an acceptable > prerequisite. This also allows us to replace raw_smp_processor_id() > with smp_processor_id() since the short circuit logic will prevent > warnings from PREEMPT_DEBUG. > > Fixes: dcc7871128e9 ("kgdb: core changes to support kdb") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > include/linux/kgdb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > index b072aeb1fd78..4d6fe87fd38f 100644 > --- a/include/linux/kgdb.h > +++ b/include/linux/kgdb.h > @@ -323,7 +323,7 @@ extern void gdbstub_exit(int status); > extern int kgdb_single_step; > extern atomic_t kgdb_active; > #define in_dbg_master() \ > - (raw_smp_processor_id() == atomic_read(&kgdb_active)) > + (irqs_disabled() && (smp_processor_id() == atomic_read(&kgdb_active))) > extern bool dbg_is_early; > extern void __init dbg_late_init(void); > extern void kgdb_panic(const char *msg); Cheers, Daniel. I assume you'll route this one via the kgdb tree? I can live with the "small window" in the arm64 for-next/core branch ;) Will
On Thu, May 07, 2020 at 09:39:30AM +0100, Will Deacon wrote: > On Wed, May 06, 2020 at 05:42:23PM +0100, Daniel Thompson wrote: > > Currently there is a small window where a badly timed migration could > > cause in_dbg_master() to spuriously return true. Specifically if we > > migrate to a new core after reading the processor id and the previous > > core takes a breakpoint then we will evaluate true if we read > > kgdb_active before we get the IPI to bring us to halt. > > > > Fix this by checking irqs_disabled() first. Interrupts are always > > disabled when we are executing the kgdb trap so this is an acceptable > > prerequisite. This also allows us to replace raw_smp_processor_id() > > with smp_processor_id() since the short circuit logic will prevent > > warnings from PREEMPT_DEBUG. > > > > Fixes: dcc7871128e9 ("kgdb: core changes to support kdb") > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > --- > > include/linux/kgdb.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > > index b072aeb1fd78..4d6fe87fd38f 100644 > > --- a/include/linux/kgdb.h > > +++ b/include/linux/kgdb.h > > @@ -323,7 +323,7 @@ extern void gdbstub_exit(int status); > > extern int kgdb_single_step; > > extern atomic_t kgdb_active; > > #define in_dbg_master() \ > > - (raw_smp_processor_id() == atomic_read(&kgdb_active)) > > + (irqs_disabled() && (smp_processor_id() == atomic_read(&kgdb_active))) > > extern bool dbg_is_early; > > extern void __init dbg_late_init(void); > > extern void kgdb_panic(const char *msg); > > Cheers, Daniel. I assume you'll route this one via the kgdb tree? I can > live with the "small window" in the arm64 for-next/core branch ;) Yes. I'll get this one applied very soon (thanks for Doug for the quick review). Daniel.
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h index b072aeb1fd78..4d6fe87fd38f 100644 --- a/include/linux/kgdb.h +++ b/include/linux/kgdb.h @@ -323,7 +323,7 @@ extern void gdbstub_exit(int status); extern int kgdb_single_step; extern atomic_t kgdb_active; #define in_dbg_master() \ - (raw_smp_processor_id() == atomic_read(&kgdb_active)) + (irqs_disabled() && (smp_processor_id() == atomic_read(&kgdb_active))) extern bool dbg_is_early; extern void __init dbg_late_init(void); extern void kgdb_panic(const char *msg);
Currently there is a small window where a badly timed migration could cause in_dbg_master() to spuriously return true. Specifically if we migrate to a new core after reading the processor id and the previous core takes a breakpoint then we will evaluate true if we read kgdb_active before we get the IPI to bring us to halt. Fix this by checking irqs_disabled() first. Interrupts are always disabled when we are executing the kgdb trap so this is an acceptable prerequisite. This also allows us to replace raw_smp_processor_id() with smp_processor_id() since the short circuit logic will prevent warnings from PREEMPT_DEBUG. Fixes: dcc7871128e9 ("kgdb: core changes to support kdb") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- include/linux/kgdb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 6a8b55ed4056ea5559ebe4f6a4b247f627870d4c -- 2.25.1