| Message ID | 1611313556-4004-1-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | Accepted |
| Commit | 93f7a6d818deef69d0ba652d46bae6fbabbf365c |
| Headers | show |
| Series | [v3] kdb: Make memory allocations more robust | expand |
Hi, On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > Currently kdb uses in_interrupt() to determine whether its library > code has been called from the kgdb trap handler or from a saner calling > context such as driver init. This approach is broken because > in_interrupt() alone isn't able to determine kgdb trap handler entry from > normal task context. This can happen during normal use of basic features > such as breakpoints and can also be trivially reproduced using: > echo g > /proc/sysrq-trigger I guess an alternative to your patch is to fully eliminate GFP_KDB. It always strikes me as a sub-optimal design to choose between GFP_ATOMIC and GFP_KERNEL like this. Presumably others must agree because otherwise I'd expect that the overall kernel would have something like "GFP_AUTOMATIC"? It doesn't feel like it'd be that hard to do something more explicit. From a quick glance: * I think kdb_defcmd() and kdb_defcmd2() are always called in response to a user typing something on the kdb command line. Those should always be GFP_ATOMIC, right? * The one call that's not in kdb_defcmd() / kdb_defcmd2() is in kdb_register_flags(). That can be called either during init time or from kdb_defcmd2(). It doesn't seem like it'd be hard to rename kdb_register_flags() to _kdb_register_flags() and add a "gfp_t flags" to the end. Then the exported kdb_register_flags() would pass GFP_KERNEL and the call from kdb_defcmd2() would pass GFP_ATOMIC: > We can improve this by adding check for in_dbg_master() instead which s/adding check/adding a check/ > explicitly determines if we are running in debugger context. > > Cc: stable@vger.kernel.org > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > > Changes in v3: > - Refined commit description and Cc: stable@vger.kernel.org. > > Changes in v2: > - Get rid of redundant in_atomic() check. > > kernel/debug/kdb/kdb_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I would leave it up to Daniel to say whether he agrees that a full removal of "GFP_KDB" would be a better solution. However, your patch clearly improves the state of things, so: Reviewed-by: Douglas Anderson <dianders@chromium.org>
Thanks Doug for your comments. On Fri, 22 Jan 2021 at 22:55, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > > > Currently kdb uses in_interrupt() to determine whether its library > > code has been called from the kgdb trap handler or from a saner calling > > context such as driver init. This approach is broken because > > in_interrupt() alone isn't able to determine kgdb trap handler entry from > > normal task context. This can happen during normal use of basic features > > such as breakpoints and can also be trivially reproduced using: > > echo g > /proc/sysrq-trigger > > I guess an alternative to your patch is to fully eliminate GFP_KDB. > It always strikes me as a sub-optimal design to choose between > GFP_ATOMIC and GFP_KERNEL like this. Presumably others must agree > because otherwise I'd expect that the overall kernel would have > something like "GFP_AUTOMATIC"? > > It doesn't feel like it'd be that hard to do something more explicit. > From a quick glance: > > * I think kdb_defcmd() and kdb_defcmd2() are always called in response > to a user typing something on the kdb command line. Those should > always be GFP_ATOMIC, right? > > * The one call that's not in kdb_defcmd() / kdb_defcmd2() is in > kdb_register_flags(). That can be called either during init time or > from kdb_defcmd2(). It doesn't seem like it'd be hard to rename > kdb_register_flags() to _kdb_register_flags() and add a "gfp_t flags" > to the end. Then the exported kdb_register_flags() would pass > GFP_KERNEL and the call from kdb_defcmd2() would pass GFP_ATOMIC: > Thanks for your suggestions. I agree with you that it's better to get rid of GFP_KDB. But I think we need to backport this fix to stable kernels as well, so IMO a minimal change like this would be better. I will rather push a seperate code refactoring patch to incorporate your suggestions. > > > We can improve this by adding check for in_dbg_master() instead which > > s/adding check/adding a check/ > Ack. If we don't have any further comments, can this be incorporated while applying this patch? > > > explicitly determines if we are running in debugger context. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > > > Changes in v3: > > - Refined commit description and Cc: stable@vger.kernel.org. > > > > Changes in v2: > > - Get rid of redundant in_atomic() check. > > > > kernel/debug/kdb/kdb_private.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > I would leave it up to Daniel to say whether he agrees that a full > removal of "GFP_KDB" would be a better solution. However, your patch > clearly improves the state of things, so: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks, Sumit
On Fri, Jan 22, 2021 at 09:25:44AM -0800, Doug Anderson wrote: > Hi, > > On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > > > Currently kdb uses in_interrupt() to determine whether its library > > code has been called from the kgdb trap handler or from a saner calling > > context such as driver init. This approach is broken because > > in_interrupt() alone isn't able to determine kgdb trap handler entry from > > normal task context. This can happen during normal use of basic features > > such as breakpoints and can also be trivially reproduced using: > > echo g > /proc/sysrq-trigger > > I guess an alternative to your patch is to fully eliminate GFP_KDB. > It always strikes me as a sub-optimal design to choose between > GFP_ATOMIC and GFP_KERNEL like this. Presumably others must agree > because otherwise I'd expect that the overall kernel would have > something like "GFP_AUTOMATIC"? > > It doesn't feel like it'd be that hard to do something more explicit. > From a quick glance: > > * I think kdb_defcmd() and kdb_defcmd2() are always called in response > to a user typing something on the kdb command line. Those should > always be GFP_ATOMIC, right? No. I'm afraid not. The kdb parser is also used to execute kernel/debug/kdb/kdb_cmds as part of the kdb initialization. This initialization happens from the init calls rather than from the kgdb trap handler code. When I first looked at Sumit's patch I had a similar reaction to you but, whilst it is clearly it's not impossible to pass flags into the kdb parser and all its subcommands, I concluded that GFP_KDB is a better approach. BTW the reason I insisted on getting rid of the in_atomic() was to make it clear that GFP_KDB discriminates between exactly two calling contexts (normal and kgdb trap handler). I was didn't want any hints that imply GFP_KDB is a (broken) implementation of something like GFP_AUTOMATIC! Daniel.
On Mon, 25 Jan 2021 at 13:48, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Jan 22, 2021 at 09:25:44AM -0800, Doug Anderson wrote: > > Hi, > > > > On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > > > > > Currently kdb uses in_interrupt() to determine whether its library > > > code has been called from the kgdb trap handler or from a saner calling > > > context such as driver init. This approach is broken because > > > in_interrupt() alone isn't able to determine kgdb trap handler entry from > > > normal task context. This can happen during normal use of basic features > > > such as breakpoints and can also be trivially reproduced using: > > > echo g > /proc/sysrq-trigger > > > > I guess an alternative to your patch is to fully eliminate GFP_KDB. > > It always strikes me as a sub-optimal design to choose between > > GFP_ATOMIC and GFP_KERNEL like this. Presumably others must agree > > because otherwise I'd expect that the overall kernel would have > > something like "GFP_AUTOMATIC"? > > > > It doesn't feel like it'd be that hard to do something more explicit. > > From a quick glance: > > > > * I think kdb_defcmd() and kdb_defcmd2() are always called in response > > to a user typing something on the kdb command line. Those should > > always be GFP_ATOMIC, right? > > No. I'm afraid not. The kdb parser is also used to execute > kernel/debug/kdb/kdb_cmds as part of the kdb initialization. This > initialization happens from the init calls rather than from the kgdb > trap handler code. > > When I first looked at Sumit's patch I had a similar reaction to you > but, whilst it is clearly it's not impossible to pass flags into the > kdb parser and all its subcommands, I concluded that GFP_KDB is a > better approach. > > BTW the reason I insisted on getting rid of the in_atomic() was to make > it clear that GFP_KDB discriminates between exactly two calling contexts > (normal and kgdb trap handler). I was didn't want any hints that imply > GFP_KDB is a (broken) implementation of something like GFP_AUTOMATIC! > Ah, I see the reasoning to keep GFP_KDB. So we don't need any further refactoring and can go ahead with this patch only. -Sumit > > Daniel.
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 7a4a181..344eb0d 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); #define kdb_task_has_cpu(p) (task_curr(p)) -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) +#define GFP_KDB (in_dbg_master() ? GFP_ATOMIC : GFP_KERNEL) extern void *debug_kmalloc(size_t size, gfp_t flags); extern void debug_kfree(void *);
Currently kdb uses in_interrupt() to determine whether its library code has been called from the kgdb trap handler or from a saner calling context such as driver init. This approach is broken because in_interrupt() alone isn't able to determine kgdb trap handler entry from normal task context. This can happen during normal use of basic features such as breakpoints and can also be trivially reproduced using: echo g > /proc/sysrq-trigger We can improve this by adding check for in_dbg_master() instead which explicitly determines if we are running in debugger context. Cc: stable@vger.kernel.org Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- Changes in v3: - Refined commit description and Cc: stable@vger.kernel.org. Changes in v2: - Get rid of redundant in_atomic() check. kernel/debug/kdb/kdb_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.7.4