mbox series

[RFC,0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

Message ID 20200605132130.1411255-1-daniel.thompson@linaro.org
Headers show
Series kgdb: Honour the kprobe blacklist when setting breakpoints | expand

Message

Daniel Thompson June 5, 2020, 1:21 p.m. UTC
kgdb has traditionally adopted a no safety rails approach to breakpoint
placement. If the debugger is commanded to place a breakpoint at an
address then it will do so even if that breakpoint results in kgdb
becoming inoperable.

A stop-the-world debugger with memory peek/poke does intrinsically
provide its operator with the means to hose their system in all manner
of exciting ways (not least because stopping-the-world is already a DoS
attack ;-) ) but the current no safety rail approach is not easy to
defend, especially given kprobes provides us with plenty of machinery to
mark parts of the kernel where breakpointing is discouraged.

This patchset introduces some safety rails by using the existing
kprobes infrastructure. It does not cover all locations where
breakpoints can cause trouble but it will definitely block off several
avenues, including the architecture specific parts that are handled by
arch_within_kprobe_blacklist().

This patch is an RFC because:

1. My workstation is still chugging through the compile testing.

2. Patch 4 needs more runtime testing.

3. The code to extract the kprobe blacklist code (patch 4 again) needs
   more review especially for its impact on arch specific code.

To be clear I do plan to do the detailed review of the kprobe blacklist
stuff but would like to check the direction of travel first since the
change is already surprisingly big and maybe there's a better way to
organise things.


Daniel.


Daniel Thompson (4):
  kgdb: Honour the kprobe blacklist when setting breakpoints
  kgdb: Use the kprobe blacklist to limit single stepping
  kgdb: Add NOKPROBE labels on the trap handler functions
  kprobes: Allow the kprobes blacklist to be compiled independently

 arch/Kconfig                            |   6 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/blacklist.c     |  37 ++++
 arch/arm/probes/kprobes/core.c          |  10 -
 arch/powerpc/kernel/Makefile            |   1 +
 arch/powerpc/kernel/kprobes-blacklist.c |  34 ++++
 arch/powerpc/kernel/kprobes.c           |   8 -
 include/asm-generic/kprobes.h           |   2 +-
 include/asm-generic/vmlinux.lds.h       |   2 +-
 include/linux/kgdb.h                    |   1 +
 include/linux/kprobes.h                 |  29 ++-
 kernel/Makefile                         |   1 +
 kernel/debug/debug_core.c               |  31 +++
 kernel/debug/gdbstub.c                  |  10 +-
 kernel/debug/kdb/kdb_bp.c               |  17 +-
 kernel/debug/kdb/kdb_main.c             |  10 +-
 kernel/kprobes.c                        | 204 +------------------
 kernel/kprobes_blacklist.c              | 260 ++++++++++++++++++++++++
 lib/Kconfig.kgdb                        |   1 +
 19 files changed, 427 insertions(+), 238 deletions(-)
 create mode 100644 arch/arm/probes/kprobes/blacklist.c
 create mode 100644 arch/powerpc/kernel/kprobes-blacklist.c
 create mode 100644 kernel/kprobes_blacklist.c

--
2.25.4

Comments

Peter Zijlstra June 5, 2020, 2:29 p.m. UTC | #1
On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:
> kgdb has traditionally adopted a no safety rails approach to breakpoint

> placement. If the debugger is commanded to place a breakpoint at an

> address then it will do so even if that breakpoint results in kgdb

> becoming inoperable.

> 

> A stop-the-world debugger with memory peek/poke does intrinsically

> provide its operator with the means to hose their system in all manner

> of exciting ways (not least because stopping-the-world is already a DoS

> attack ;-) ) but the current no safety rail approach is not easy to

> defend, especially given kprobes provides us with plenty of machinery to

> mark parts of the kernel where breakpointing is discouraged.

> 

> This patchset introduces some safety rails by using the existing

> kprobes infrastructure. It does not cover all locations where

> breakpoints can cause trouble but it will definitely block off several

> avenues, including the architecture specific parts that are handled by

> arch_within_kprobe_blacklist().

> 

> This patch is an RFC because:

> 

> 1. My workstation is still chugging through the compile testing.

> 

> 2. Patch 4 needs more runtime testing.

> 

> 3. The code to extract the kprobe blacklist code (patch 4 again) needs

>    more review especially for its impact on arch specific code.

> 

> To be clear I do plan to do the detailed review of the kprobe blacklist

> stuff but would like to check the direction of travel first since the

> change is already surprisingly big and maybe there's a better way to

> organise things.


Thanks for doing these patches, esp 1-3 look very good to me.

I've taken the liberty to bounce the entire set to Masami-San, who is
the kprobes maintainer for comments as well.
Peter Zijlstra June 5, 2020, 2:44 p.m. UTC | #2
On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > placement. If the debugger is commanded to place a breakpoint at an

> > address then it will do so even if that breakpoint results in kgdb

> > becoming inoperable.

> > 

> > A stop-the-world debugger with memory peek/poke does intrinsically

> > provide its operator with the means to hose their system in all manner

> > of exciting ways (not least because stopping-the-world is already a DoS

> > attack ;-) ) but the current no safety rail approach is not easy to

> > defend, especially given kprobes provides us with plenty of machinery to

> > mark parts of the kernel where breakpointing is discouraged.

> > 

> > This patchset introduces some safety rails by using the existing

> > kprobes infrastructure. It does not cover all locations where

> > breakpoints can cause trouble but it will definitely block off several

> > avenues, including the architecture specific parts that are handled by

> > arch_within_kprobe_blacklist().

> > 

> > This patch is an RFC because:

> > 

> > 1. My workstation is still chugging through the compile testing.

> > 

> > 2. Patch 4 needs more runtime testing.

> > 

> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> >    more review especially for its impact on arch specific code.

> > 

> > To be clear I do plan to do the detailed review of the kprobe blacklist

> > stuff but would like to check the direction of travel first since the

> > change is already surprisingly big and maybe there's a better way to

> > organise things.

> 

> Thanks for doing these patches, esp 1-3 look very good to me.

> 

> I've taken the liberty to bounce the entire set to Masami-San, who is

> the kprobes maintainer for comments as well.


OK, after having had a second look, one thing we can perhaps address
with the last patch, or perhaps on top of that, is extending the
kprobes_blacklist() with data regions.

Because these patches only exclude kgdb from setting breakpoints on
code; data breakpoints do not match what we do with
arch_build_bp_info().
Daniel Thompson June 8, 2020, 12:43 p.m. UTC | #3
On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > placement. If the debugger is commanded to place a breakpoint at an

> > address then it will do so even if that breakpoint results in kgdb

> > becoming inoperable.

> > 

> > A stop-the-world debugger with memory peek/poke does intrinsically

> > provide its operator with the means to hose their system in all manner

> > of exciting ways (not least because stopping-the-world is already a DoS

> > attack ;-) ) but the current no safety rail approach is not easy to

> > defend, especially given kprobes provides us with plenty of machinery to

> > mark parts of the kernel where breakpointing is discouraged.

> > 

> > This patchset introduces some safety rails by using the existing

> > kprobes infrastructure. It does not cover all locations where

> > breakpoints can cause trouble but it will definitely block off several

> > avenues, including the architecture specific parts that are handled by

> > arch_within_kprobe_blacklist().

> > 

> > This patch is an RFC because:

> > 

> > 1. My workstation is still chugging through the compile testing.

> > 

> > 2. Patch 4 needs more runtime testing.

> > 

> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> >    more review especially for its impact on arch specific code.

> > 

> > To be clear I do plan to do the detailed review of the kprobe blacklist

> > stuff but would like to check the direction of travel first since the

> > change is already surprisingly big and maybe there's a better way to

> > organise things.

> 

> Thanks for doing these patches, esp 1-3 look very good to me.

> 

> I've taken the liberty to bounce the entire set to Masami-San, who is

> the kprobes maintainer for comments as well.


Not a liberty... leaving out Masami-san was an oversight on my part.
Thanks for connecting!


Daniel.
Daniel Thompson June 8, 2020, 1:50 p.m. UTC | #4
On Fri, Jun 05, 2020 at 04:44:57PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:

> > On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > > placement. If the debugger is commanded to place a breakpoint at an

> > > address then it will do so even if that breakpoint results in kgdb

> > > becoming inoperable.

> > > 

> > > A stop-the-world debugger with memory peek/poke does intrinsically

> > > provide its operator with the means to hose their system in all manner

> > > of exciting ways (not least because stopping-the-world is already a DoS

> > > attack ;-) ) but the current no safety rail approach is not easy to

> > > defend, especially given kprobes provides us with plenty of machinery to

> > > mark parts of the kernel where breakpointing is discouraged.

> > > 

> > > This patchset introduces some safety rails by using the existing

> > > kprobes infrastructure. It does not cover all locations where

> > > breakpoints can cause trouble but it will definitely block off several

> > > avenues, including the architecture specific parts that are handled by

> > > arch_within_kprobe_blacklist().

> > > 

> > > This patch is an RFC because:

> > > 

> > > 1. My workstation is still chugging through the compile testing.

> > > 

> > > 2. Patch 4 needs more runtime testing.

> > > 

> > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> > >    more review especially for its impact on arch specific code.

> > > 

> > > To be clear I do plan to do the detailed review of the kprobe blacklist

> > > stuff but would like to check the direction of travel first since the

> > > change is already surprisingly big and maybe there's a better way to

> > > organise things.

> > 

> > Thanks for doing these patches, esp 1-3 look very good to me.

> > 

> > I've taken the liberty to bounce the entire set to Masami-San, who is

> > the kprobes maintainer for comments as well.

> 

> OK, after having had a second look, one thing we can perhaps address

> with the last patch, or perhaps on top of that, is extending the

> kprobes_blacklist() with data regions.

> 

> Because these patches only exclude kgdb from setting breakpoints on

> code; data breakpoints do not match what we do with

> arch_build_bp_info().


Right, my patches will only affect the code paths where kgdb sets
software breakpoints.

In principle h/w breakpoints, whether they are code or data, should be
able to rely on hw_breakpoint_arch_parse() and any errors from the hw
breakpoint API will propagate into the kgdb core and do the right
thing.

In practice it looks like kgdb/x86/hw_breakpoint have plumbed together
in a manner that circumvents the parse (and therefore#
arch_build_bp_info() never runs). Rather the h/w/ break problem using
the kprobe blacklist I think we could just fix these code paths.

The following is 100% untested (not even compile) and I copied a line
or two from register_perf_hw_breakpoint() without checking what they
do ;-). Nevertheless I hope it gives a clear idea about what I am
talking about! If this was developed into a "real" patch then I think
dbg_release_bp_slot() should perhaps be replaced with a better API that
doesn't bypass the checks rather than solving everything in kgdb.c .


Daniel.


diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index c44fe7d8d9a4..64ac0ee9b55c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -223,11 +223,12 @@ static void kgdb_correct_hw_break(void)
 		hw_breakpoint_restore();
 }
 
-static int hw_break_reserve_slot(int breakno)
+static int kgdb_register_hw_break(int breakno)
 {
 	int cpu;
 	int cnt = 0;
 	struct perf_event **pevent;
+        struct arch_hw_breakpoint hw = { };
 
 	if (dbg_is_early)
 		return 0;
@@ -237,6 +238,11 @@ static int hw_break_reserve_slot(int breakno)
 		pevent = per_cpu_ptr(breakinfo[breakno].pev, cpu);
 		if (dbg_reserve_bp_slot(*pevent))
 			goto fail;
+		if (hw_breakpoint_arch_parse(*pevent, &(*pevent)->attr, hw)) {
+			cnt++;
+			goto fail;
+		}
+		(*pevent)->hw.info = hw;
 	}
 
 	return 0;
@@ -361,7 +367,7 @@ kgdb_set_hw_break(unsigned long addr, int len, enum kgdb_bptype bptype)
 		return -1;
 	}
 	breakinfo[i].addr = addr;
-	if (hw_break_reserve_slot(i)) {
+	if (kgdb_register_hw_break(i)) {
 		breakinfo[i].addr = 0;
 		return -1;
 	}
Masami Hiramatsu June 11, 2020, 12:42 p.m. UTC | #5
On Fri, 5 Jun 2020 16:29:53 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > placement. If the debugger is commanded to place a breakpoint at an

> > address then it will do so even if that breakpoint results in kgdb

> > becoming inoperable.

> > 

> > A stop-the-world debugger with memory peek/poke does intrinsically

> > provide its operator with the means to hose their system in all manner

> > of exciting ways (not least because stopping-the-world is already a DoS

> > attack ;-) ) but the current no safety rail approach is not easy to

> > defend, especially given kprobes provides us with plenty of machinery to

> > mark parts of the kernel where breakpointing is discouraged.

> > 

> > This patchset introduces some safety rails by using the existing

> > kprobes infrastructure. It does not cover all locations where

> > breakpoints can cause trouble but it will definitely block off several

> > avenues, including the architecture specific parts that are handled by

> > arch_within_kprobe_blacklist().

> > 

> > This patch is an RFC because:

> > 

> > 1. My workstation is still chugging through the compile testing.

> > 

> > 2. Patch 4 needs more runtime testing.

> > 

> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> >    more review especially for its impact on arch specific code.

> > 

> > To be clear I do plan to do the detailed review of the kprobe blacklist

> > stuff but would like to check the direction of travel first since the

> > change is already surprisingly big and maybe there's a better way to

> > organise things.

> 

> Thanks for doing these patches, esp 1-3 look very good to me.

> 

> I've taken the liberty to bounce the entire set to Masami-San, who is

> the kprobes maintainer for comments as well.


Thanks Peter to Cc me.

Reusing kprobe blacklist is good to me as far as it doesn't expand it
only for kgdb. For example, if a function which can cause a recursive
trap issue only when the kgdb puts a breakpoint, it should be covered
by kgdb blacklist, or we should make a "noinstr-list" including
both :)

Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants
to use the "kprobes blacklist", we should make CONFIG_KGDB depending on
CONFIG_KPROBES. Or, (as I pointed) we should make independent "noinstr-list"
and use it from both.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>
Daniel Thompson June 11, 2020, 2:32 p.m. UTC | #6
On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:
> On Fri, 5 Jun 2020 16:29:53 +0200

> Peter Zijlstra <peterz@infradead.org> wrote:

> 

> > On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > > placement. If the debugger is commanded to place a breakpoint at an

> > > address then it will do so even if that breakpoint results in kgdb

> > > becoming inoperable.

> > > 

> > > A stop-the-world debugger with memory peek/poke does intrinsically

> > > provide its operator with the means to hose their system in all manner

> > > of exciting ways (not least because stopping-the-world is already a DoS

> > > attack ;-) ) but the current no safety rail approach is not easy to

> > > defend, especially given kprobes provides us with plenty of machinery to

> > > mark parts of the kernel where breakpointing is discouraged.

> > > 

> > > This patchset introduces some safety rails by using the existing

> > > kprobes infrastructure. It does not cover all locations where

> > > breakpoints can cause trouble but it will definitely block off several

> > > avenues, including the architecture specific parts that are handled by

> > > arch_within_kprobe_blacklist().

> > > 

> > > This patch is an RFC because:

> > > 

> > > 1. My workstation is still chugging through the compile testing.

> > > 

> > > 2. Patch 4 needs more runtime testing.

> > > 

> > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> > >    more review especially for its impact on arch specific code.

> > > 

> > > To be clear I do plan to do the detailed review of the kprobe blacklist

> > > stuff but would like to check the direction of travel first since the

> > > change is already surprisingly big and maybe there's a better way to

> > > organise things.

> > 

> > Thanks for doing these patches, esp 1-3 look very good to me.

> > 

> > I've taken the liberty to bounce the entire set to Masami-San, who is

> > the kprobes maintainer for comments as well.

> 

> Thanks Peter to Cc me.

> 

> Reusing kprobe blacklist is good to me as far as it doesn't expand it

> only for kgdb. For example, if a function which can cause a recursive

> trap issue only when the kgdb puts a breakpoint, it should be covered

> by kgdb blacklist, or we should make a "noinstr-list" including

> both :)


Recursion is what focuses the mind but I don't think we'd need
recursion for there to be problems.

For example taking a kprobe trap whilst executing the kgdb trap handler
(or vice versa) is already likely to be fragile and is almost certainly
untested on most or all architectures. Further if I understood Peter's
original nudge correctly then, in addition, x86 plans to explicitly
prohibit this anyway.

On other words I think there will only be one blacklist.


> Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants

> to use the "kprobes blacklist", we should make CONFIG_KGDB depending on

> CONFIG_KPROBES.


Some of the architectures currently have kgdb support but do not have
kprobes...


> Or, (as I pointed) we should make independent "noinstr-list"

> and use it from both.


This sounds like this wouldn't really be a functional change over
what I have proposed. More like augmenting it with a massive symbol
rename (and maybe a little bit of extra code movement in the headers
to give us linux/noinstr.h).

Taking my cues from things like set_fs() I originally decided to keep
away from such a big rename ;-) .

Personally I'm open to a rename. I could write PATCH 4/4 assuming a
rename will come (e.g. different naming for new files and Kconfig
options) and follow that with an automatically generated
rename patch (or patches).


Daniel.
Masami Hiramatsu June 12, 2020, 10:13 a.m. UTC | #7
On Thu, 11 Jun 2020 15:32:40 +0100
Daniel Thompson <daniel.thompson@linaro.org> wrote:

> On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:

> > On Fri, 5 Jun 2020 16:29:53 +0200

> > Peter Zijlstra <peterz@infradead.org> wrote:

> > 

> > > On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > > > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > > > placement. If the debugger is commanded to place a breakpoint at an

> > > > address then it will do so even if that breakpoint results in kgdb

> > > > becoming inoperable.

> > > > 

> > > > A stop-the-world debugger with memory peek/poke does intrinsically

> > > > provide its operator with the means to hose their system in all manner

> > > > of exciting ways (not least because stopping-the-world is already a DoS

> > > > attack ;-) ) but the current no safety rail approach is not easy to

> > > > defend, especially given kprobes provides us with plenty of machinery to

> > > > mark parts of the kernel where breakpointing is discouraged.

> > > > 

> > > > This patchset introduces some safety rails by using the existing

> > > > kprobes infrastructure. It does not cover all locations where

> > > > breakpoints can cause trouble but it will definitely block off several

> > > > avenues, including the architecture specific parts that are handled by

> > > > arch_within_kprobe_blacklist().

> > > > 

> > > > This patch is an RFC because:

> > > > 

> > > > 1. My workstation is still chugging through the compile testing.

> > > > 

> > > > 2. Patch 4 needs more runtime testing.

> > > > 

> > > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> > > >    more review especially for its impact on arch specific code.

> > > > 

> > > > To be clear I do plan to do the detailed review of the kprobe blacklist

> > > > stuff but would like to check the direction of travel first since the

> > > > change is already surprisingly big and maybe there's a better way to

> > > > organise things.

> > > 

> > > Thanks for doing these patches, esp 1-3 look very good to me.

> > > 

> > > I've taken the liberty to bounce the entire set to Masami-San, who is

> > > the kprobes maintainer for comments as well.

> > 

> > Thanks Peter to Cc me.

> > 

> > Reusing kprobe blacklist is good to me as far as it doesn't expand it

> > only for kgdb. For example, if a function which can cause a recursive

> > trap issue only when the kgdb puts a breakpoint, it should be covered

> > by kgdb blacklist, or we should make a "noinstr-list" including

> > both :)

> 

> Recursion is what focuses the mind but I don't think we'd need

> recursion for there to be problems.

> 

> For example taking a kprobe trap whilst executing the kgdb trap handler

> (or vice versa) is already likely to be fragile and is almost certainly

> untested on most or all architectures.


Ah, OK. Actually, on x86 that is not a problem (it can handle recursive int3
if software handles it correctly), but other arch may not accept it.
Hmm, then using NOKPROBE_SYMBOL() is reasonable.

> Further if I understood Peter's

> original nudge correctly then, in addition, x86 plans to explicitly

> prohibit this anyway.

> 

> On other words I think there will only be one blacklist.


Agreed.

> > Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants

> > to use the "kprobes blacklist", we should make CONFIG_KGDB depending on

> > CONFIG_KPROBES.

> 

> Some of the architectures currently have kgdb support but do not have

> kprobes...


"depends on KPROBES if HAVE_KPROBES" ? :-)

(Anyway, it is a good chance to port kprobe on such arch...)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>
Daniel Thompson June 12, 2020, 11:04 a.m. UTC | #8
On Fri, Jun 12, 2020 at 07:13:49PM +0900, Masami Hiramatsu wrote:
> On Thu, 11 Jun 2020 15:32:40 +0100

> Daniel Thompson <daniel.thompson@linaro.org> wrote:

> 

> > On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:

> > > On Fri, 5 Jun 2020 16:29:53 +0200

> > > Peter Zijlstra <peterz@infradead.org> wrote:

> > > 

> > > > On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:

> > > > > kgdb has traditionally adopted a no safety rails approach to breakpoint

> > > > > placement. If the debugger is commanded to place a breakpoint at an

> > > > > address then it will do so even if that breakpoint results in kgdb

> > > > > becoming inoperable.

> > > > > 

> > > > > A stop-the-world debugger with memory peek/poke does intrinsically

> > > > > provide its operator with the means to hose their system in all manner

> > > > > of exciting ways (not least because stopping-the-world is already a DoS

> > > > > attack ;-) ) but the current no safety rail approach is not easy to

> > > > > defend, especially given kprobes provides us with plenty of machinery to

> > > > > mark parts of the kernel where breakpointing is discouraged.

> > > > > 

> > > > > This patchset introduces some safety rails by using the existing

> > > > > kprobes infrastructure. It does not cover all locations where

> > > > > breakpoints can cause trouble but it will definitely block off several

> > > > > avenues, including the architecture specific parts that are handled by

> > > > > arch_within_kprobe_blacklist().

> > > > > 

> > > > > This patch is an RFC because:

> > > > > 

> > > > > 1. My workstation is still chugging through the compile testing.

> > > > > 

> > > > > 2. Patch 4 needs more runtime testing.

> > > > > 

> > > > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs

> > > > >    more review especially for its impact on arch specific code.

> > > > > 

> > > > > To be clear I do plan to do the detailed review of the kprobe blacklist

> > > > > stuff but would like to check the direction of travel first since the

> > > > > change is already surprisingly big and maybe there's a better way to

> > > > > organise things.

> > > > 

> > > > Thanks for doing these patches, esp 1-3 look very good to me.

> > > > 

> > > > I've taken the liberty to bounce the entire set to Masami-San, who is

> > > > the kprobes maintainer for comments as well.

> > > 

> > > Thanks Peter to Cc me.

> > > 

> > > Reusing kprobe blacklist is good to me as far as it doesn't expand it

> > > only for kgdb. For example, if a function which can cause a recursive

> > > trap issue only when the kgdb puts a breakpoint, it should be covered

> > > by kgdb blacklist, or we should make a "noinstr-list" including

> > > both :)

> > 

> > Recursion is what focuses the mind but I don't think we'd need

> > recursion for there to be problems.

> > 

> > For example taking a kprobe trap whilst executing the kgdb trap handler

> > (or vice versa) is already likely to be fragile and is almost certainly

> > untested on most or all architectures.

> 

> Ah, OK. Actually, on x86 that is not a problem (it can handle recursive int3

> if software handles it correctly), but other arch may not accept it.

> Hmm, then using NOKPROBE_SYMBOL() is reasonable.

> 

> > Further if I understood Peter's

> > original nudge correctly then, in addition, x86 plans to explicitly

> > prohibit this anyway.

> > 

> > On other words I think there will only be one blacklist.

> 

> Agreed.

> 

> > > Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants

> > > to use the "kprobes blacklist", we should make CONFIG_KGDB depending on

> > > CONFIG_KPROBES.

> > 

> > Some of the architectures currently have kgdb support but do not have

> > kprobes...

> 

> "depends on KPROBES if HAVE_KPROBES" ? :-)


I'm personally be OK with something like:

#ifndef CONFIG_KGDB_ALLOW_UNSAFE_BREAKPOINTS
  if (within_kprobe_blacklist())
    ...
#endif

And then setup Kconfig so that KGDB_ALLOW_UNSAFE_BREAKPOINTS
defaults to n and add a extra check to put a warning in dmesg
that breakpoints are disabled.


> (Anyway, it is a good chance to port kprobe on such arch...)


I like kprobes very much... but not quite enough to want to
implement it on architectures I don't use ;-).


Daniel.