[PATCH 0/2] Fix KGDB to work with CONFIG_DEBUG_RODATA using kprobe API

[PATCH 0/2] Fix KGDB to work with CONFIG_DEBUG_RODATA using kprobe API

[Jason Wessel]

The inability to use software breakpoints on a kernel built with
CONFIG_DEBUG_RODATA has been a problem for quite a few years.  The
kprobes API has been working around this limitation for a long
time. This patch set changes the debug_core to use the kprobe
breakpoint API directly for a kernel compiled with CONFIG_DEBUG_RODATA.

Comments are welcome of course.

Thanks,
Jason.

---
Jason Wessel (2):
      kgdb,debug_core: pass the breakpoint struct instead of address and memory
      kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

 drivers/misc/kgdbts.c     |   13 ------
 include/linux/kgdb.h      |    7 ++-
 kernel/debug/debug_core.c |   93 ++++++++++++++++++++++++++++++--------------
 3 files changed, 67 insertions(+), 46 deletions(-)

[PATCH 1/2] kgdb,debug_core: pass the breakpoint struct instead of address and memory

[Jason Wessel]

There is extra state information that needs to be exposed in the
kgdb_bpt structure for tracking whether or not a breakpoint was
installed via a kprobe or via a probe_kernel_write().  In order to
access the structure it needs to be passed to the
kgdb_arch_set_breakpoint() and kgdb_arch_remove_breakpoint().

A future commit adds the logic for the kprobe breakpoint type.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 include/linux/kgdb.h      |    4 +-
 kernel/debug/debug_core.c |   53 ++++++++++++++++++++------------------------
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index fa39183..e5d689c 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -207,8 +207,8 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);
 
 /* Optional functions. */
 extern int kgdb_validate_break_address(unsigned long addr);
-extern int kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr);
-extern int kgdb_arch_remove_breakpoint(unsigned long addr, char *bundle);
+extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);
+extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);
 
 /**
  *	kgdb_arch_late - Perform any architecture specific initalization.
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 3f88a45..a7e52ca 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -161,37 +161,39 @@ early_param("nokgdbroundup", opt_nokgdbroundup);
  * Weak aliases for breakpoint management,
  * can be overriden by architectures when needed:
  */
-int __weak kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr)
+int __weak kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 {
 	int err;
 
-	err = probe_kernel_read(saved_instr, (char *)addr, BREAK_INSTR_SIZE);
+	err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
+				BREAK_INSTR_SIZE);
 	if (err)
 		return err;
-
-	return probe_kernel_write((char *)addr, arch_kgdb_ops.gdb_bpt_instr,
-				  BREAK_INSTR_SIZE);
+	err = probe_kernel_write((char *)bpt->bpt_addr,
+				 arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
+	return err;
 }
 
-int __weak kgdb_arch_remove_breakpoint(unsigned long addr, char *bundle)
+int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 {
-	return probe_kernel_write((char *)addr,
-				  (char *)bundle, BREAK_INSTR_SIZE);
+	return probe_kernel_write((char *)bpt->bpt_addr,
+				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
 }
 
 int __weak kgdb_validate_break_address(unsigned long addr)
 {
-	char tmp_variable[BREAK_INSTR_SIZE];
+	struct kgdb_bkpt tmp;
 	int err;
-	/* Validate setting the breakpoint and then removing it.  In the
+	/* Validate setting the breakpoint and then removing it.  If the
 	 * remove fails, the kernel needs to emit a bad message because we
 	 * are deep trouble not being able to put things back the way we
 	 * found them.
 	 */
-	err = kgdb_arch_set_breakpoint(addr, tmp_variable);
+	tmp.bpt_addr = addr;
+	err = kgdb_arch_set_breakpoint(&tmp);
 	if (err)
 		return err;
-	err = kgdb_arch_remove_breakpoint(addr, tmp_variable);
+	err = kgdb_arch_remove_breakpoint(&tmp);
 	if (err)
 		printk(KERN_ERR "KGDB: Critical breakpoint error, kernel "
 		   "memory destroyed at: %lx", addr);
@@ -235,7 +237,6 @@ static void kgdb_flush_swbreak_addr(unsigned long addr)
  */
 int dbg_activate_sw_breakpoints(void)
 {
-	unsigned long addr;
 	int error;
 	int ret = 0;
 	int i;
@@ -244,16 +245,15 @@ int dbg_activate_sw_breakpoints(void)
 		if (kgdb_break[i].state != BP_SET)
 			continue;
 
-		addr = kgdb_break[i].bpt_addr;
-		error = kgdb_arch_set_breakpoint(addr,
-				kgdb_break[i].saved_instr);
+		error = kgdb_arch_set_breakpoint(&kgdb_break[i]);
 		if (error) {
 			ret = error;
-			printk(KERN_INFO "KGDB: BP install failed: %lx", addr);
+			printk(KERN_INFO "KGDB: BP install failed: %lx",
+			       kgdb_break[i].bpt_addr);
 			continue;
 		}
 
-		kgdb_flush_swbreak_addr(addr);
+		kgdb_flush_swbreak_addr(kgdb_break[i].bpt_addr);
 		kgdb_break[i].state = BP_ACTIVE;
 	}
 	return ret;
@@ -302,7 +302,6 @@ int dbg_set_sw_break(unsigned long addr)
 
 int dbg_deactivate_sw_breakpoints(void)
 {
-	unsigned long addr;
 	int error;
 	int ret = 0;
 	int i;
@@ -310,15 +309,14 @@ int dbg_deactivate_sw_breakpoints(void)
 	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
 		if (kgdb_break[i].state != BP_ACTIVE)
 			continue;
-		addr = kgdb_break[i].bpt_addr;
-		error = kgdb_arch_remove_breakpoint(addr,
-					kgdb_break[i].saved_instr);
+		error = kgdb_arch_remove_breakpoint(&kgdb_break[i]);
 		if (error) {
-			printk(KERN_INFO "KGDB: BP remove failed: %lx\n", addr);
+			printk(KERN_INFO "KGDB: BP remove failed: %lx\n",
+			       kgdb_break[i].bpt_addr);
 			ret = error;
 		}
 
-		kgdb_flush_swbreak_addr(addr);
+		kgdb_flush_swbreak_addr(kgdb_break[i].bpt_addr);
 		kgdb_break[i].state = BP_SET;
 	}
 	return ret;
@@ -352,7 +350,6 @@ int kgdb_isremovedbreak(unsigned long addr)
 
 int dbg_remove_all_break(void)
 {
-	unsigned long addr;
 	int error;
 	int i;
 
@@ -360,12 +357,10 @@ int dbg_remove_all_break(void)
 	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
 		if (kgdb_break[i].state != BP_ACTIVE)
 			goto setundefined;
-		addr = kgdb_break[i].bpt_addr;
-		error = kgdb_arch_remove_breakpoint(addr,
-				kgdb_break[i].saved_instr);
+		error = kgdb_arch_remove_breakpoint(&kgdb_break[i]);
 		if (error)
 			printk(KERN_ERR "KGDB: breakpoint remove failed: %lx\n",
-			   addr);
+			       kgdb_break[i].bpt_addr);
 setundefined:
 		kgdb_break[i].state = BP_UNDEFINED;
 	}
-- 
1.7.5.4

[PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Jason Wessel]

There has long been a limitation using software breakpoints with a
kernel compiled with CONFIG_DEBUG_RODATA.  The kprobe breakpoint code
has its own text_poke() function which accommodates writing a
breakpoint into a read-only page.  The debug_core can make use of the
text_poke() capabilities by using the kprobes API, specifically
arch_arm_kprobe() and arch_disarm_kprobe().  For now it is safe to use
a single statically allocated kprobe structure to call the kprobes API
because the debug_core breakpoint API is only used when the kernel is
in the debug state.

The debug_core will first attempt to use the traditional
probe_kernel_write(), and next try using a kprobe breakpoint.  The
kgdb test suite was updated to run all the software breakpoint tests
when using a kernel with built with CONFIG_DEBUG_RODATA.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/misc/kgdbts.c     |   13 -------------
 include/linux/kgdb.h      |    3 ++-
 kernel/debug/debug_core.c |   40 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 3f7ad83..672b4a5 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -940,19 +940,6 @@ static void kgdbts_run_tests(void)
 		run_nmi_sleep_test(nmi_sleep);
 	}
 
-#ifdef CONFIG_DEBUG_RODATA
-	/* Until there is an api to write to read-only text segments, use
-	 * HW breakpoints for the remainder of any tests, else print a
-	 * failure message if hw breakpoints do not work.
-	 */
-	if (!(arch_kgdb_ops.flags & KGDB_HW_BREAKPOINT && hwbreaks_ok)) {
-		eprintk("kgdbts: HW breakpoints do not work,"
-			"skipping remaining tests\n");
-		return;
-	}
-	force_hwbrks = 1;
-#endif /* CONFIG_DEBUG_RODATA */
-
 	/* If the do_fork test is run it will be the last test that is
 	 * executed because a kernel thread will be spawned at the very
 	 * end to unregister the debug hooks.
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index e5d689c..48f17d2 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -63,7 +63,8 @@ enum kgdb_bptype {
 	BP_HARDWARE_BREAKPOINT,
 	BP_WRITE_WATCHPOINT,
 	BP_READ_WATCHPOINT,
-	BP_ACCESS_WATCHPOINT
+	BP_ACCESS_WATCHPOINT,
+	BP_KPROBE_BREAKPOINT,
 };
 
 enum kgdb_bpstate {
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index a7e52ca..9051844 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -49,6 +49,7 @@
 #include <linux/smp.h>
 #include <linux/mm.h>
 #include <linux/rcupdate.h>
+#include <linux/kprobes.h>
 
 #include <asm/cacheflush.h>
 #include <asm/byteorder.h>
@@ -108,6 +109,13 @@ module_param(kgdbreboot, int, 0644);
 static struct kgdb_bkpt		kgdb_break[KGDB_MAX_BREAKPOINTS] = {
 	[0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED }
 };
+/*
+ * probe_write_tmp is used to r/w breakpoints with the kprobe
+ * interface, it is not protected for reentrancy
+ */
+#if defined(CONFIG_KPROBES) && defined(CONFIG_DEBUG_RODATA)
+static struct kprobe probe_write_tmp;
+#endif /* CONFIG_KPROBES && CONFIG_DEBUG_RODATA */
 
 /*
  * The CPU# of the active CPU, or -1 if none:
@@ -165,17 +173,48 @@ int __weak kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 {
 	int err;
 
+	bpt->type = BP_BREAKPOINT;
 	err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
 				BREAK_INSTR_SIZE);
 	if (err)
 		return err;
 	err = probe_kernel_write((char *)bpt->bpt_addr,
 				 arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
+#if defined(CONFIG_KPROBES) && defined(CONFIG_DEBUG_RODATA)
+	if (!err)
+		return err;
+	probe_write_tmp.addr = (kprobe_opcode_t *)bpt->bpt_addr;
+	arch_arm_kprobe(&probe_write_tmp);
+	err = probe_kernel_read(&probe_write_tmp.opcode, (char *)bpt->bpt_addr,
+				BREAK_INSTR_SIZE);
+	if (err)
+		return err;
+	if (memcmp(&probe_write_tmp.opcode, arch_kgdb_ops.gdb_bpt_instr,
+		   BREAK_INSTR_SIZE))
+		return -EINVAL;
+	bpt->type = BP_KPROBE_BREAKPOINT;
+#endif /* CONFIG_KPROBES && CONFIG_DEBUG_RODATA */
 	return err;
 }
 
 int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 {
+#if defined(CONFIG_KPROBES) && defined(CONFIG_DEBUG_RODATA)
+	int err;
+
+	if (bpt->type != BP_KPROBE_BREAKPOINT)
+		goto knl_write;
+	probe_write_tmp.addr = (kprobe_opcode_t *)bpt->bpt_addr;
+	memcpy(&probe_write_tmp.opcode, bpt->saved_instr, BREAK_INSTR_SIZE);
+	arch_disarm_kprobe(&probe_write_tmp);
+	err = probe_kernel_read(&probe_write_tmp.opcode, (char *)bpt->bpt_addr,
+				BREAK_INSTR_SIZE);
+	if (err ||
+	    memcmp(&probe_write_tmp.opcode, bpt->saved_instr, BREAK_INSTR_SIZE))
+		goto knl_write;
+	return err;
+knl_write:
+#endif /* CONFIG_KPROBES && CONFIG_DEBUG_RODATA */
 	return probe_kernel_write((char *)bpt->bpt_addr,
 				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
 }
@@ -294,7 +333,6 @@ int dbg_set_sw_break(unsigned long addr)
 		return -E2BIG;
 
 	kgdb_break[breakno].state = BP_SET;
-	kgdb_break[breakno].type = BP_BREAKPOINT;
 	kgdb_break[breakno].bpt_addr = addr;
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Masami Hiramatsu]

(2012/03/22 2:55), Jason Wessel wrote:
> There has long been a limitation using software breakpoints with a
> kernel compiled with CONFIG_DEBUG_RODATA.  The kprobe breakpoint code
> has its own text_poke() function which accommodates writing a
> breakpoint into a read-only page.  The debug_core can make use of the
> text_poke() capabilities by using the kprobes API, specifically
> arch_arm_kprobe() and arch_disarm_kprobe().  For now it is safe to use
> a single statically allocated kprobe structure to call the kprobes API
> because the debug_core breakpoint API is only used when the kernel is
> in the debug state.

You might misunderstand it. arch_*_kprobe() are not open APIs.
Those are kprobes internal APIs (which means that those functions
should be used only by kprobes).


> The debug_core will first attempt to use the traditional
> probe_kernel_write(), and next try using a kprobe breakpoint.  The
> kgdb test suite was updated to run all the software breakpoint tests
> when using a kernel with built with CONFIG_DEBUG_RODATA.
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

Nak.

[...]
> @@ -165,17 +173,48 @@ int __weak kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
>  {
>  	int err;
>  
> +	bpt->type = BP_BREAKPOINT;
>  	err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
>  				BREAK_INSTR_SIZE);
>  	if (err)
>  		return err;
>  	err = probe_kernel_write((char *)bpt->bpt_addr,
>  				 arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
> +#if defined(CONFIG_KPROBES) && defined(CONFIG_DEBUG_RODATA)
> +	if (!err)
> +		return err;
> +	probe_write_tmp.addr = (kprobe_opcode_t *)bpt->bpt_addr;
> +	arch_arm_kprobe(&probe_write_tmp);

No, please don't use kprobes internal function this way, because
you can't ensure that the arch_arm_kprobe() has no side-effect.

Why don't you use text_poke()? I see that the text_poke()
is only for x86, but you already have arch/x86/kernel/kgdb.c for
making your own wrapper function.

> +	err = probe_kernel_read(&probe_write_tmp.opcode, (char *)bpt->bpt_addr,
> +				BREAK_INSTR_SIZE);
> +	if (err)
> +		return err;
> +	if (memcmp(&probe_write_tmp.opcode, arch_kgdb_ops.gdb_bpt_instr,
> +		   BREAK_INSTR_SIZE))
> +		return -EINVAL;
> +	bpt->type = BP_KPROBE_BREAKPOINT;
> +#endif /* CONFIG_KPROBES && CONFIG_DEBUG_RODATA */
>  	return err;
>  }
>  
>  int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
>  {
> +#if defined(CONFIG_KPROBES) && defined(CONFIG_DEBUG_RODATA)
> +	int err;
> +
> +	if (bpt->type != BP_KPROBE_BREAKPOINT)
> +		goto knl_write;
> +	probe_write_tmp.addr = (kprobe_opcode_t *)bpt->bpt_addr;
> +	memcpy(&probe_write_tmp.opcode, bpt->saved_instr, BREAK_INSTR_SIZE);
> +	arch_disarm_kprobe(&probe_write_tmp);

Ditto.

> +	err = probe_kernel_read(&probe_write_tmp.opcode, (char *)bpt->bpt_addr,
> +				BREAK_INSTR_SIZE);
> +	if (err ||
> +	    memcmp(&probe_write_tmp.opcode, bpt->saved_instr, BREAK_INSTR_SIZE))
> +		goto knl_write;
> +	return err;
> +knl_write:
> +#endif /* CONFIG_KPROBES && CONFIG_DEBUG_RODATA */
>  	return probe_kernel_write((char *)bpt->bpt_addr,
>  				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
>  }
> @@ -294,7 +333,6 @@ int dbg_set_sw_break(unsigned long addr)
>  		return -E2BIG;
>  
>  	kgdb_break[breakno].state = BP_SET;
> -	kgdb_break[breakno].type = BP_BREAKPOINT;
>  	kgdb_break[breakno].bpt_addr = addr;
>  
>  	return 0;

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Jason Wessel]

On 03/21/2012 09:53 PM, Masami Hiramatsu wrote:
> (2012/03/22 2:55), Jason Wessel wrote:
>> There has long been a limitation using software breakpoints with a
>> kernel compiled with CONFIG_DEBUG_RODATA.  The kprobe breakpoint code
>> has its own text_poke() function which accommodates writing a
>> breakpoint into a read-only page.  The debug_core can make use of the
>> text_poke() capabilities by using the kprobes API, specifically
>> arch_arm_kprobe() and arch_disarm_kprobe().  For now it is safe to use
>> a single statically allocated kprobe structure to call the kprobes API
>> because the debug_core breakpoint API is only used when the kernel is
>> in the debug state.
> 
> You might misunderstand it. arch_*_kprobe() are not open APIs.
> Those are kprobes internal APIs (which means that those functions
> should be used only by kprobes).
> 


I was looking for an interface that solved the problem, without having
to use text_poke directly which is arch specific.  Eventually I would
like to use the kprobes high level API, but it cannot not be used
without taking a mutex presently.  This is a separate problem to deal
with at a later time, because the generic use of kprobes would be
aimed at having robust single stepping.

> 
>> The debug_core will first attempt to use the traditional
>> probe_kernel_write(), and next try using a kprobe breakpoint.  The
>> kgdb test suite was updated to run all the software breakpoint tests
>> when using a kernel with built with CONFIG_DEBUG_RODATA.
>>
>> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> 
> Nak.
> 
> No, please don't use kprobes internal function this way, because
> you can't ensure that the arch_arm_kprobe() has no side-effect.
> 
> Why don't you use text_poke()? I see that the text_poke()
> is only for x86, but you already have arch/x86/kernel/kgdb.c for
> making your own wrapper function.

I will use the arch specific provision to override the
kgdb_arch_set_breakpoint() and use the text_poke() directly.

Eventually I would like to use the same software breakpoint
reservation system as kprobes, and that would happen if kgdb ever
starts using kprobes for single stepping.  A few years back we solved
the problem for hardware breakpoints reservations where the kernel
debugger, perf, ptrace, and kprobes all play nice.


Thanks,
Jason.

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Masami Hiramatsu]

(2012/03/22 20:57), Jason Wessel wrote:
> On 03/21/2012 09:53 PM, Masami Hiramatsu wrote:
>> (2012/03/22 2:55), Jason Wessel wrote:
>>> There has long been a limitation using software breakpoints with a
>>> kernel compiled with CONFIG_DEBUG_RODATA.  The kprobe breakpoint code
>>> has its own text_poke() function which accommodates writing a
>>> breakpoint into a read-only page.  The debug_core can make use of the
>>> text_poke() capabilities by using the kprobes API, specifically
>>> arch_arm_kprobe() and arch_disarm_kprobe().  For now it is safe to use
>>> a single statically allocated kprobe structure to call the kprobes API
>>> because the debug_core breakpoint API is only used when the kernel is
>>> in the debug state.
>>
>> You might misunderstand it. arch_*_kprobe() are not open APIs.
>> Those are kprobes internal APIs (which means that those functions
>> should be used only by kprobes).
>>
>
>
> I was looking for an interface that solved the problem, without having
> to use text_poke directly which is arch specific.  Eventually I would
> like to use the kprobes high level API, but it cannot not be used
> without taking a mutex presently.  This is a separate problem to deal
> with at a later time, because the generic use of kprobes would be
> aimed at having robust single stepping.

I see.

>>> The debug_core will first attempt to use the traditional
>>> probe_kernel_write(), and next try using a kprobe breakpoint.  The
>>> kgdb test suite was updated to run all the software breakpoint tests
>>> when using a kernel with built with CONFIG_DEBUG_RODATA.
>>>
>>> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
>>
>> Nak.
>>
>> No, please don't use kprobes internal function this way, because
>> you can't ensure that the arch_arm_kprobe() has no side-effect.
>>
>> Why don't you use text_poke()? I see that the text_poke()
>> is only for x86, but you already have arch/x86/kernel/kgdb.c for
>> making your own wrapper function.
>
> I will use the arch specific provision to override the
> kgdb_arch_set_breakpoint() and use the text_poke() directly.

Thanks! that's what I meant. You can use __weak attribute.

> Eventually I would like to use the same software breakpoint
> reservation system as kprobes, and that would happen if kgdb ever
> starts using kprobes for single stepping.

Yeah, as far as I can see, current kprobes design assumes that
there is no other part uses sw breakpoint in kernel. But there
is kgdb now.
I think kprobes can share the sw breakpoint instrumentation and
single stepping with kgdb.

Thank you,



-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Jason Wessel]

On 03/23/2012 09:08 AM, Masami Hiramatsu wrote:
> (2012/03/22 20:57), Jason Wessel wrote:
>> I will use the arch specific provision to override the
>> kgdb_arch_set_breakpoint() and use the text_poke() directly.
> 
> Thanks! that's what I meant. You can use __weak attribute.
> 

I created and tested a patch yesterday which is show below.  I will
post a new series at some point soon which addresses this problem as
well as a number of problems found with the kgdb test suite.

Cheers,
Jason.


Subject: [PATCH] x86,kgdb: End DEBUG_RODATA limitation using text_poke()

There has long been a limitation using software breakpoints with a
kernel compiled with CONFIG_DEBUG_RODATA.  The kprobes code has long
used the text_poke() function which accommodates writing a breakpoint
into a read-only page.  The x86 arch can override the default
breakpoint install remove routines and make use of the text_poke()
code that comes from the x86 alternatives.

The x86 arch will first attempt to use the traditional
probe_kernel_write(), and next try using a the text_poke() function.
The break point install method is tracked such that the correct break
point removal routine will get called later on.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 arch/x86/kernel/kgdb.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/kgdb.h   |    3 ++-
 2 files changed, 48 insertions(+), 1 deletion(-)

--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -63,7 +63,8 @@ enum kgdb_bptype {
 	BP_HARDWARE_BREAKPOINT,
 	BP_WRITE_WATCHPOINT,
 	BP_READ_WATCHPOINT,
-	BP_ACCESS_WATCHPOINT
+	BP_ACCESS_WATCHPOINT,
+	BP_POKE_BREAKPOINT,
 };
 
 enum kgdb_bpstate {
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -742,6 +742,52 @@ void kgdb_arch_set_pc(struct pt_regs *re
 	regs->ip = ip;
 }
 
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+	int err;
+	char opc[BREAK_INSTR_SIZE];
+
+	bpt->type = BP_BREAKPOINT;
+	err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
+				BREAK_INSTR_SIZE);
+	if (err)
+		return err;
+	err = probe_kernel_write((char *)bpt->bpt_addr,
+				 arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
+#ifdef CONFIG_DEBUG_RODATA
+	if (!err)
+		return err;
+	text_poke((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
+		  BREAK_INSTR_SIZE);
+	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
+	if (err)
+		return err;
+	if (memcmp(opc, arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE))
+		return -EINVAL;
+	bpt->type = BP_POKE_BREAKPOINT;
+#endif /* CONFIG_DEBUG_RODATA */
+	return err;
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+#ifdef CONFIG_DEBUG_RODATA
+	int err;
+	char opc[BREAK_INSTR_SIZE];
+
+	if (bpt->type != BP_POKE_BREAKPOINT)
+		goto knl_write;
+	text_poke((void *)bpt->bpt_addr, bpt->saved_instr, BREAK_INSTR_SIZE);
+	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
+	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
+		goto knl_write;
+	return err;
+knl_write:
+#endif /* CONFIG_DEBUG_RODATA */
+	return probe_kernel_write((char *)bpt->bpt_addr,
+				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
+}
+
 struct kgdb_arch arch_kgdb_ops = {
 	/* Breakpoint instruction: */
 	.gdb_bpt_instr		= { 0xcc },

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Masami Hiramatsu]

(2012/03/23 23:38), Jason Wessel wrote:
> On 03/23/2012 09:08 AM, Masami Hiramatsu wrote:
>> (2012/03/22 20:57), Jason Wessel wrote:
>>> I will use the arch specific provision to override the
>>> kgdb_arch_set_breakpoint() and use the text_poke() directly.
>>
>> Thanks! that's what I meant. You can use __weak attribute.
>>
> 
> I created and tested a patch yesterday which is show below.  I will
> post a new series at some point soon which addresses this problem as
> well as a number of problems found with the kgdb test suite.

Yeah, that's better.

BTW, I'm not sure the policy of kgdb about mutex, but it seems
that you need to hold a text_mutex when you call the text_poke()
since it uses a fixmap page-area for mapping read-only text page
to writable page. So, without locking (at least ensuring no one
using) text_mutex, it seems not be safe. (some other code may be
trying to change the code by using same fixmap pages)

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kgdb,debug_core,kgdbts: End DEBUG_RODATA limitation using kprobe breakpoints

[Jason Wessel]

On 03/26/2012 04:46 AM, Masami Hiramatsu wrote:
> (2012/03/23 23:38), Jason Wessel wrote:
>> On 03/23/2012 09:08 AM, Masami Hiramatsu wrote:
>>> (2012/03/22 20:57), Jason Wessel wrote:
>>>> I will use the arch specific provision to override the
>>>> kgdb_arch_set_breakpoint() and use the text_poke() directly.
>>>
>>> Thanks! that's what I meant. You can use __weak attribute.
>>>
>>
>> I created and tested a patch yesterday which is show below.  I will
>> post a new series at some point soon which addresses this problem as
>> well as a number of problems found with the kgdb test suite.
> 
> Yeah, that's better.
> 
> BTW, I'm not sure the policy of kgdb about mutex, but it seems
> that you need to hold a text_mutex when you call the text_poke()
> since it uses a fixmap page-area for mapping read-only text page
> to writable page. So, without locking (at least ensuring no one
> using) text_mutex, it seems not be safe. (some other code may be
> trying to change the code by using same fixmap pages)

Thank you very much for the advice.  I had run the kgdb mutex
validation which checks for mutex's taken any time I change the kgdb
code and it passed.  However, this did not check for incorrect usage
where the debug core should really be taking a mutex to prevent
corruption.  The comments in the text_poke code clearly indicate a
caller must hold the text_mutex().

I started looking through all the code that uses text_mutex and what
it actually protects.  It looked like it is probably possible to make
things re-entrant in order to deal with the case where debugger changes
a location with a fixmap when kernel execution is stopped.  I am not
convinced this is a good idea, given complexity of the code, vs the
small number of users and the likely hood of interference being on the
low side.  For now, I am not going to pursue making any kind of
changes with fix map or the text_mutex protected regions.  Today there
are only 3 users of the text_mutex, SMP alternatives, jump labels
updates, and kprobes, so the risk for collision is fairly low.

At the point in time that the collisions become a real problem, such
as kgdb starting to use kprobes directly, changing some code for
special re-entrance considerations after using the kernel debugger
might get considered again.  Until then, I will use the simple
approach of checking the mutex and use text_poke() if it is not locked
when the normal kernel execution is stopped on all cores.

The delta to the previous patch is shown below.

Thanks,
Jason.


diff -u b/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
--- b/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -757,6 +757,12 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 #ifdef CONFIG_DEBUG_RODATA
 	if (!err)
 		return err;
+	/* 
+	 * It is safe to call text_poke() because normal kernel execution
+	 * is stopped on all cores, so long as the text_mutex is not locked.
+	 */
+	if (mutex_is_locked(&text_mutex)
+		return -EBUSY;
 	text_poke((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
 		  BREAK_INSTR_SIZE);
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
@@ -777,6 +783,12 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 
 	if (bpt->type != BP_POKE_BREAKPOINT)
 		goto knl_write;
+	/* 
+	 * It is safe to call text_poke() because normal kernel execution
+	 * is stopped on all cores, so long as the text_mutex is not locked.
+	 */
+	if (mutex_is_locked(&text_mutex)
+		goto knl_write;
 	text_poke((void *)bpt->bpt_addr, bpt->saved_instr, BREAK_INSTR_SIZE);
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))