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

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

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