[PATCH v3] kdb: Make memory allocations more robust

[PATCH v3] kdb: Make memory allocations more robust

[Sumit Garg]

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(-)

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 *);
-- 
2.7.4

Re: [PATCH v3] kdb: Make memory allocations more robust

[Doug Anderson]

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>

Re: [PATCH v3] kdb: Make memory allocations more robust

[Sumit Garg]

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

Re: [PATCH v3] kdb: Make memory allocations more robust

[Daniel Thompson]

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.

Re: [PATCH v3] kdb: Make memory allocations more robust

[Sumit Garg]

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.