Re: [PATCH v2 3/3] kgdb: Add NOKPROBE labels on the trap handler functions

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Jul 21, 2020 at 02:22:08PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 20, 2020 at 1:13 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Fri, Jul 17, 2020 at 03:39:58PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson
> > > <daniel.thompson@linaro.org> wrote:
> > > >
> > > > Currently kgdb honours the kprobe blocklist but doesn't place its own
> > > > trap handling code on the list. Add labels to discourage attempting to
> > > > use kgdb to debug itself.
> > > >
> > > > These changes do not make it impossible to provoke recursive trapping
> > > > since they do not cover all the calls that can be made on kgdb's entry
> > > > logic. However going much further whilst we are sharing the kprobe
> > > > blocklist risks reducing the capabilities of kprobe and this would be a
> > > > bad trade off (especially so given kgdb's users are currently conditioned
> > > > to avoid recursive traps).
> > > >
> > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > > ---
> > > >  kernel/debug/debug_core.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > >
> > > I could just be missing something, but...
> > >
> > > I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions
> > > that kgdb happens to call, but I'm not quite sure I understand why all
> > > of the kdb / kgdb code itself shouldn't be in the blocklist.  I
> > > certainly don't object to the functions you added to the blocklist, I
> > > guess I'm must trying to understand why it's a bad idea to add more or
> > > how you came up with the list of functions that you did.
> >
> > Relatively early in the trap handler execution (just after we bring the
> > other CPUs to a halt) all breakpoints are replaced with the original
> > opcodes. Therefore I only marked up functions that run between the trap
> > firing and the breakpoints being removed (and also between the
> > breakpoints being reinstated and trap exit).
> 
> Ah, OK!  Could that be added to the commit message?

Will do.
 
> Also, shouldn't you mark kgdb_arch_set_breakpoint()?  What about
> dbg_activate_sw_breakpoints()?  I haven't gone and extensively
> searched, but those two jump out to me as ones that were missed.

Agree. I think I over-focusses on the entry path. I will review the
exit path more closely.

> I suppose that means that if someone tried to set a breakpoint on a
> kgdb function that wasn't one of the ones that you listed then the
> system would happily report that the breakpoint has been set (no error
> given) but that the breakpoint would just have no effect?  It wouldn't
> crash (which is good), it just wouldn't detect that the breakpoint was
> useless.

Assuming the kgdb function is used exclusively from the trap handler
then this is correct.

> However, if these were in the NOKPROBE_SYMBOL then you'd get
> a nice error message.  Is there no way we could use a linker script to
> just mark everything using a linker script or somesuch?

You'd still get odd effects with library functions that are used inside
and outside the debugger (which can be breakpointed but don't trigger
inside kgdb). Arguably the effect is clearer to users if they can see
kdb/kgdb functions behaving the same way as library functions. It's odd
but it won't promote false expectations from users.


Daniel.