Hi, On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg <sumit.garg@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx> > --- > > Changes in v3: > - Refined commit description and Cc: stable@xxxxxxxxxxxxxxx. > > 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@xxxxxxxxxxxx>
