Re: kgdb test suite failure

[Jason Wessel <jason.wessel@windriver.com>]

Alexander Beregalov wrote:
> 2008/5/20 Jason Wessel <jason.wessel@windriver.com>:
>> Alexander Beregalov wrote:
>>> Hi
>>> I tried to run the latest git kernel and got the following error.
>>>
>>> See an attachment for full dmesg.
>>>
>>> kgdbts: ERROR PUT: end of test buffer on 'do_fork_test' line 5
>>> expected OK got $E02#a7
>>> ------------[ cut here ]------------
>>> WARNING: at drivers/misc/kgdbts.c:721 run_simple_test+0x1de/0x20e()
>>> Modules linked in:
>>> Pid: 6, comm: khelper Not tainted 2.6.26-rc1-00279-g28a4acb #12
>>> [<c011a00a>] warn_on_slowpath+0x41/0x6c
>>> [<c011aa77>] ? vprintk+0x3e7/0x407
>>> [<c0254cf4>] ? number+0x10d/0x1cf
>>> [<c012d3da>] ? sched_clock_cpu+0x70/0x88
>>> [<c011aaac>] ? printk+0x15/0x17
>>>
>> Is this a problem you can reproduce every time?  Basically what line 5
> Yes, it happened every time when kernel booted, even before init.
> 
>> of the test does is to write a value into the register struct to rewind
>> the PC after hitting a breakpoint such that the instruction will be
>> executed again.   This is stored in memory which will be used for a
>> context switch back to the process.   The test also replaces the
>> original instruction in memory.  In this case the memory write failed.
>> This will certainly be fatal to the operation of the kernel and
>> stability would be called into question if the kernel doesn't just crash
>> and oops in strange ways immediately.
>>
>> I looked in your dmesg log and noticed:
>> [   21.027632] Testing CPA: undo c035d000-c044e000
>> [   21.083029] Testing CPA: write protecting again
>> [   21.297278] kgdbts: ERROR PUT: end of test buffer on 'do_fork_test'
>> line 5 expected OK got $E02#a7
>>
>>
>> I looked through the kernel source in the tip of the tree for the
>> "Testing CPA: write protecting again".  It seems this code is only used
>> when CONFIG_DEBUG_RODATA is set.  Perhaps this option is mutually
>> exclusive of the use of kgdb?  Today kgdb assumes it can write anywhere
>> without any kind of special concession for page protection bits.


I duplicated the problem by trying the kernel out with the
CONFIG_DEBUG_RODATA, which appears to only be implemented on the x86
architecture.  I am not exactly certain what the right level of fix
should be in this case, or if it should be fixed at all.  It seems
that the probe_kernel_write should have a force option to force
writing to a read-only memory page (meaning it will be
unprotected/re-protected), or there should be another function to deal
with writes that are deemed to be critical which calls the
probe_kernel_write() from with in kgdb.

It is an interesting problem, but the first inclination is not to fix
it at all because debugging read-only text sections is generally a job
for hardware breakpoints.  The kgdb test suite fell into the trap of
trying to debug a function call that was in a read-only page using
software breakpoints with trap instructions.

An RFC type patch follows which addresses the problem, but in my
opinion not in a terribly clean way...  Perhaps other folks will
comment on a different or better way to approach the problem or chime
in on if it should be fixed at all.  Certainly the test suite could be
changed to use HW breakpoints for the case where the text segments are
going to be read only as an example.

Another approach I considered was to add a global option to kgdb which
causes it to force write to read only pages.  This could be used by
the kgdb test suite as well as by a "human" via a debugger to change
the setting as needed.  How often you might want to set a software
breakpoint in a read-only page remains to be seen, but it does seem
like something that is marginally useful.

Jason.


===RFC only to illustrate a work around===

---
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 60bcb5b..e4d0039 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -737,7 +737,7 @@ static int change_page_attr_set_clr(unsigned long addr, int numpages,
 	/*
 	 * Check whether we really changed something:
 	 */
-	if (!cpa.flushtlb)
+	if (!cpa.flushtlb || in_atomic())
 		goto out;
 
 	/*
diff --git a/mm/maccess.c b/mm/maccess.c
index ac40796..7609fb9 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -4,6 +4,7 @@
 #include <linux/uaccess.h>
 #include <linux/module.h>
 #include <linux/mm.h>
+#include <asm/cacheflush.h>
 
 /**
  * probe_kernel_read(): safely attempt to read from a location
@@ -42,11 +43,24 @@ EXPORT_SYMBOL_GPL(probe_kernel_read);
 long probe_kernel_write(void *dst, void *src, size_t size)
 {
 	long ret;
+#ifdef CONFIG_X86
+	unsigned int level;
+	pte_t *pte = lookup_address((unsigned long)dst, &level);
+	int unprotect = !pte_write(*pte);
+#endif
 	mm_segment_t old_fs = get_fs();
 
 	set_fs(KERNEL_DS);
 	pagefault_disable();
+#ifdef CONFIG_X86
+	if (unprotect)
+		set_memory_rw(((unsigned long)dst) & PAGE_MASK, 1);
+#endif
 	ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
+#ifdef CONFIG_X86
+	if (unprotect)
+		set_memory_ro(((unsigned long)dst) & PAGE_MASK, 1);
+#endif
 	pagefault_enable();
 	set_fs(old_fs);