Messages in this thread | | | From | Doug Anderson <> | Date | Fri, 22 Jan 2021 09:25:44 -0800 | Subject | Re: [PATCH v3] kdb: Make memory allocations more robust |
| |
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>
| |