* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-24 23:58 ` Douglas Anderson 0 siblings, 0 replies; 10+ messages in thread From: Douglas Anderson @ 2015-08-24 23:58 UTC (permalink / raw) To: linux-arm-kernel If we're in kgdb then the machine is already stopped. Trying to stop it again will cause us to try to sleep, which is not allowed while in kgdb. To avoid this problem, only stop the machine when we're not in kgdb. Reported-by: Aapo Vienamo <avienamo@nvidia.com> Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- arch/arm/kernel/patch.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c index 69bda1a..abf30ec 100644 --- a/arch/arm/kernel/patch.c +++ b/arch/arm/kernel/patch.c @@ -1,5 +1,6 @@ #include <linux/kernel.h> #include <linux/spinlock.h> +#include <linux/kgdb.h> #include <linux/kprobes.h> #include <linux/mm.h> #include <linux/stop_machine.h> @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int insn) .insn = insn, }; - stop_machine(patch_text_stop_machine, &patch, NULL); + /* Stop machine before patching; but not if in the debugger */ + if (unlikely(in_dbg_master())) + patch_text_stop_machine(&patch); + else + stop_machine(patch_text_stop_machine, &patch, NULL); } -- 2.5.0.457.gab17608 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-24 23:58 ` Douglas Anderson 0 siblings, 0 replies; 10+ messages in thread From: Douglas Anderson @ 2015-08-24 23:58 UTC (permalink / raw) To: Kees Cook, Nicolas Pitre Cc: Aapo Vienamo, Douglas Anderson, linux, rabin, tixy, wangnan0, linux-arm-kernel, linux-kernel If we're in kgdb then the machine is already stopped. Trying to stop it again will cause us to try to sleep, which is not allowed while in kgdb. To avoid this problem, only stop the machine when we're not in kgdb. Reported-by: Aapo Vienamo <avienamo@nvidia.com> Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- arch/arm/kernel/patch.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c index 69bda1a..abf30ec 100644 --- a/arch/arm/kernel/patch.c +++ b/arch/arm/kernel/patch.c @@ -1,5 +1,6 @@ #include <linux/kernel.h> #include <linux/spinlock.h> +#include <linux/kgdb.h> #include <linux/kprobes.h> #include <linux/mm.h> #include <linux/stop_machine.h> @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int insn) .insn = insn, }; - stop_machine(patch_text_stop_machine, &patch, NULL); + /* Stop machine before patching; but not if in the debugger */ + if (unlikely(in_dbg_master())) + patch_text_stop_machine(&patch); + else + stop_machine(patch_text_stop_machine, &patch, NULL); } -- 2.5.0.457.gab17608 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger 2015-08-24 23:58 ` Douglas Anderson @ 2015-08-25 0:19 ` Stephen Boyd -1 siblings, 0 replies; 10+ messages in thread From: Stephen Boyd @ 2015-08-25 0:19 UTC (permalink / raw) To: linux-arm-kernel On 08/24/2015 04:58 PM, Douglas Anderson wrote: > If we're in kgdb then the machine is already stopped. Trying to stop > it again will cause us to try to sleep, which is not allowed while in > kgdb. To avoid this problem, only stop the machine when we're not in > kgdb. > > Reported-by: Aapo Vienamo <avienamo@nvidia.com> > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Can you add the backtrace? > arch/arm/kernel/patch.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c > index 69bda1a..abf30ec 100644 > --- a/arch/arm/kernel/patch.c > +++ b/arch/arm/kernel/patch.c > @@ -1,5 +1,6 @@ > #include <linux/kernel.h> > #include <linux/spinlock.h> > +#include <linux/kgdb.h> > #include <linux/kprobes.h> > #include <linux/mm.h> > #include <linux/stop_machine.h> > @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int insn) > .insn = insn, > }; > > - stop_machine(patch_text_stop_machine, &patch, NULL); > + /* Stop machine before patching; but not if in the debugger */ > + if (unlikely(in_dbg_master())) > + patch_text_stop_machine(&patch); > + else > + stop_machine(patch_text_stop_machine, &patch, NULL); > } Perhaps it would be better to add a different function for the kgdb call site? Then it's explicit what's going on without us having to figure out when in_dbg_master() is true. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-25 0:19 ` Stephen Boyd 0 siblings, 0 replies; 10+ messages in thread From: Stephen Boyd @ 2015-08-25 0:19 UTC (permalink / raw) To: Douglas Anderson Cc: Kees Cook, Nicolas Pitre, tixy, wangnan0, linux, linux-kernel, Aapo Vienamo, rabin, linux-arm-kernel On 08/24/2015 04:58 PM, Douglas Anderson wrote: > If we're in kgdb then the machine is already stopped. Trying to stop > it again will cause us to try to sleep, which is not allowed while in > kgdb. To avoid this problem, only stop the machine when we're not in > kgdb. > > Reported-by: Aapo Vienamo <avienamo@nvidia.com> > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Can you add the backtrace? > arch/arm/kernel/patch.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c > index 69bda1a..abf30ec 100644 > --- a/arch/arm/kernel/patch.c > +++ b/arch/arm/kernel/patch.c > @@ -1,5 +1,6 @@ > #include <linux/kernel.h> > #include <linux/spinlock.h> > +#include <linux/kgdb.h> > #include <linux/kprobes.h> > #include <linux/mm.h> > #include <linux/stop_machine.h> > @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int insn) > .insn = insn, > }; > > - stop_machine(patch_text_stop_machine, &patch, NULL); > + /* Stop machine before patching; but not if in the debugger */ > + if (unlikely(in_dbg_master())) > + patch_text_stop_machine(&patch); > + else > + stop_machine(patch_text_stop_machine, &patch, NULL); > } Perhaps it would be better to add a different function for the kgdb call site? Then it's explicit what's going on without us having to figure out when in_dbg_master() is true. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger 2015-08-25 0:19 ` Stephen Boyd @ 2015-08-25 16:50 ` Kees Cook -1 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2015-08-25 16:50 UTC (permalink / raw) To: linux-arm-kernel On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 08/24/2015 04:58 PM, Douglas Anderson wrote: >> >> If we're in kgdb then the machine is already stopped. Trying to stop >> it again will cause us to try to sleep, which is not allowed while in >> kgdb. To avoid this problem, only stop the machine when we're not in >> kgdb. >> >> Reported-by: Aapo Vienamo <avienamo@nvidia.com> >> Suggested-by: Kees Cook <keescook@chromium.org> I actually suggested using in_atomic_preempt_off() which is I think a better catch-all. Could you use that instead, please? -Kees >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- > > > Can you add the backtrace? > >> arch/arm/kernel/patch.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c >> index 69bda1a..abf30ec 100644 >> --- a/arch/arm/kernel/patch.c >> +++ b/arch/arm/kernel/patch.c >> @@ -1,5 +1,6 @@ >> #include <linux/kernel.h> >> #include <linux/spinlock.h> >> +#include <linux/kgdb.h> >> #include <linux/kprobes.h> >> #include <linux/mm.h> >> #include <linux/stop_machine.h> >> @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int >> insn) >> .insn = insn, >> }; >> - stop_machine(patch_text_stop_machine, &patch, NULL); >> + /* Stop machine before patching; but not if in the debugger */ >> + if (unlikely(in_dbg_master())) >> + patch_text_stop_machine(&patch); >> + else >> + stop_machine(patch_text_stop_machine, &patch, NULL); >> } > > > Perhaps it would be better to add a different function for the kgdb call > site? Then it's explicit what's going on without us having to figure out > when in_dbg_master() is true. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-25 16:50 ` Kees Cook 0 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2015-08-25 16:50 UTC (permalink / raw) To: Stephen Boyd Cc: Douglas Anderson, Nicolas Pitre, Jon Medhurst, wangnan0, Russell King - ARM Linux, LKML, Aapo Vienamo, Rabin Vincent, linux-arm-kernel@lists.infradead.org On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 08/24/2015 04:58 PM, Douglas Anderson wrote: >> >> If we're in kgdb then the machine is already stopped. Trying to stop >> it again will cause us to try to sleep, which is not allowed while in >> kgdb. To avoid this problem, only stop the machine when we're not in >> kgdb. >> >> Reported-by: Aapo Vienamo <avienamo@nvidia.com> >> Suggested-by: Kees Cook <keescook@chromium.org> I actually suggested using in_atomic_preempt_off() which is I think a better catch-all. Could you use that instead, please? -Kees >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- > > > Can you add the backtrace? > >> arch/arm/kernel/patch.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c >> index 69bda1a..abf30ec 100644 >> --- a/arch/arm/kernel/patch.c >> +++ b/arch/arm/kernel/patch.c >> @@ -1,5 +1,6 @@ >> #include <linux/kernel.h> >> #include <linux/spinlock.h> >> +#include <linux/kgdb.h> >> #include <linux/kprobes.h> >> #include <linux/mm.h> >> #include <linux/stop_machine.h> >> @@ -124,6 +125,9 @@ void __kprobes patch_text(void *addr, unsigned int >> insn) >> .insn = insn, >> }; >> - stop_machine(patch_text_stop_machine, &patch, NULL); >> + /* Stop machine before patching; but not if in the debugger */ >> + if (unlikely(in_dbg_master())) >> + patch_text_stop_machine(&patch); >> + else >> + stop_machine(patch_text_stop_machine, &patch, NULL); >> } > > > Perhaps it would be better to add a different function for the kgdb call > site? Then it's explicit what's going on without us having to figure out > when in_dbg_master() is true. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger 2015-08-25 16:50 ` Kees Cook @ 2015-08-25 19:40 ` Doug Anderson -1 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2015-08-25 19:40 UTC (permalink / raw) To: linux-arm-kernel Kees, On Tue, Aug 25, 2015 at 9:50 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 08/24/2015 04:58 PM, Douglas Anderson wrote: >>> >>> If we're in kgdb then the machine is already stopped. Trying to stop >>> it again will cause us to try to sleep, which is not allowed while in >>> kgdb. To avoid this problem, only stop the machine when we're not in >>> kgdb. >>> >>> Reported-by: Aapo Vienamo <avienamo@nvidia.com> >>> Suggested-by: Kees Cook <keescook@chromium.org> > > I actually suggested using in_atomic_preempt_off() which is I think a > better catch-all. Could you use that instead, please? I chose not to use that because (I think) in_atomic_preempt_off() doesn't guarantee that stop_machine() is not needed. Said another way: if in_atomic_preempt_off() returns true then we can't sleep on the current CPU. ...but other CPUs may be running. In this case technically we need to call stop_machine(), but we'd need to do it in a way that didn't require sleeping. In the case of kgdb we know that the machine is stopped and we're atomic too. ...so we could certainly fix kgdb by checking for "atomic", but that would mean there would be other places where we'd get no warning and (I think) invalid behavior. Stephen Boyd suggested that I just change this so that we expose a new function and that makes everything a bit cleaner. I'll try that. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-25 19:40 ` Doug Anderson 0 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2015-08-25 19:40 UTC (permalink / raw) To: Kees Cook Cc: Stephen Boyd, Nicolas Pitre, Jon Medhurst, wangnan0, Russell King - ARM Linux, LKML, Aapo Vienamo, Rabin Vincent, linux-arm-kernel@lists.infradead.org Kees, On Tue, Aug 25, 2015 at 9:50 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 08/24/2015 04:58 PM, Douglas Anderson wrote: >>> >>> If we're in kgdb then the machine is already stopped. Trying to stop >>> it again will cause us to try to sleep, which is not allowed while in >>> kgdb. To avoid this problem, only stop the machine when we're not in >>> kgdb. >>> >>> Reported-by: Aapo Vienamo <avienamo@nvidia.com> >>> Suggested-by: Kees Cook <keescook@chromium.org> > > I actually suggested using in_atomic_preempt_off() which is I think a > better catch-all. Could you use that instead, please? I chose not to use that because (I think) in_atomic_preempt_off() doesn't guarantee that stop_machine() is not needed. Said another way: if in_atomic_preempt_off() returns true then we can't sleep on the current CPU. ...but other CPUs may be running. In this case technically we need to call stop_machine(), but we'd need to do it in a way that didn't require sleeping. In the case of kgdb we know that the machine is stopped and we're atomic too. ...so we could certainly fix kgdb by checking for "atomic", but that would mean there would be other places where we'd get no warning and (I think) invalid behavior. Stephen Boyd suggested that I just change this so that we expose a new function and that makes everything a bit cleaner. I'll try that. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM: probes: Don't stop the machine if we're in the debugger 2015-08-25 0:19 ` Stephen Boyd @ 2015-08-25 22:08 ` Doug Anderson -1 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2015-08-25 22:08 UTC (permalink / raw) To: linux-arm-kernel Hi, On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > Can you add the backtrace? Good point. Done. > Perhaps it would be better to add a different function for the kgdb call > site? Then it's explicit what's going on without us having to figure out > when in_dbg_master() is true. Actually, there was already a function to patch the text directly without stopping the machine. :) Adding a bit of extra noise here to point to my new patch since it has a very different title than this one (so might be hard to find). See <https://patchwork.kernel.org/patch/7073441/>. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: probes: Don't stop the machine if we're in the debugger @ 2015-08-25 22:08 ` Doug Anderson 0 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2015-08-25 22:08 UTC (permalink / raw) To: Stephen Boyd Cc: Kees Cook, Nicolas Pitre, Jon Medhurst, wangnan0, Russell King, linux-kernel@vger.kernel.org, Aapo Vienamo, Rabin Vincent, linux-arm-kernel@lists.infradead.org Hi, On Mon, Aug 24, 2015 at 5:19 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > Can you add the backtrace? Good point. Done. > Perhaps it would be better to add a different function for the kgdb call > site? Then it's explicit what's going on without us having to figure out > when in_dbg_master() is true. Actually, there was already a function to patch the text directly without stopping the machine. :) Adding a bit of extra noise here to point to my new patch since it has a very different title than this one (so might be hard to find). See <https://patchwork.kernel.org/patch/7073441/>. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-08-25 22:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-24 23:58 [PATCH] ARM: probes: Don't stop the machine if we're in the debugger Douglas Anderson 2015-08-24 23:58 ` Douglas Anderson 2015-08-25 0:19 ` Stephen Boyd 2015-08-25 0:19 ` Stephen Boyd 2015-08-25 16:50 ` Kees Cook 2015-08-25 16:50 ` Kees Cook 2015-08-25 19:40 ` Doug Anderson 2015-08-25 19:40 ` Doug Anderson 2015-08-25 22:08 ` Doug Anderson 2015-08-25 22:08 ` Doug Anderson
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.