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.