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