linux-kernel - Re: [PATCH 18/28] powerpc,kgdb: Introduce low level trap catching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B7AF321.8060008@windriver.com>
Date:	Tue, 16 Feb 2010 13:33:53 -0600
From:	Jason Wessel <jason.wessel@...driver.com>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
CC:	linux-kernel@...r.kernel.org, kgdb-bugreport@...ts.sourceforge.net,
	mingo@...e.hu, Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH 18/28] powerpc,kgdb: Introduce low level trap catching

Benjamin Herrenschmidt wrote:
> On Fri, 2010-02-12 at 16:35 -0600, Jason Wessel wrote:
>
>   
>> @@ -115,7 +116,9 @@ void kgdb_roundup_cpus(unsigned long flags)
>>  /* KGDB functions to use existing PowerPC64 hooks. */
>>  static int kgdb_debugger(struct pt_regs *regs)
>>  {
>> -	return kgdb_handle_exception(0, computeSignal(TRAP(regs)), 0, regs);
>> +	if (kgdb_handle_exception(1, computeSignal(TRAP(regs)), DIE_OOPS, regs))
>> +		return 0;
>> +	return 1;
>>  }
>>     
>
> I'm no fan of logic inversions like that but I suppose you are trying to
> fit into existing hooks. However, I'd rather then do:
>
> 	return !kgdb...
>   

I agree and I made the change to return !kgdb...


>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -809,12 +809,19 @@ void __kprobes program_check_exception(struct pt_regs *regs)
>>  		return;
>>  	}
>>  	if (reason & REASON_TRAP) {
>> +
>> +#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
>> +		if (debugger_bpt(regs))
>> +			return;
>> +#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
>>  		/* trap exception */
>>  		if (notify_die(DIE_BPT, "breakpoint", regs, 5, 5, SIGTRAP)
>>  				== NOTIFY_STOP)
>>  			return;
>> +#ifndef CONFIG_KGDB_LOW_LEVEL_TRAP
>>  		if (debugger_bpt(regs))
>>  			return;
>> +#endif /* ! CONFIG_KGDB_LOW_LEVEL_TRAP */
>>  
>>  		if (!(regs->msr & MSR_PR) &&  /* not user-mode */
>>  		    report_bug(regs->nip, regs) == BUG_TRAP_TYPE_WARN) {
>>
>>     
> No firm objection, but it -is- a bit ugly... Should we just
> unconditionally move the debugger_bpt() early on with a comment about
> why we do so ? Is there any drawback you can think of ?
>
>   

I took a peek at xmon, and it suffers from the same problem where you
can place a breakpoint in any part of rcu_lock, notify_die, or
atomic_notifier_call_chain and meet with recursive faults.  I also
checked that xmon appears to correctly return so as to continue if the
exception was not intended for xmon.

The reason I had not just moved the code block previously is that I was
not looking to break anything such as xmon, which is the the only other
user of this function.

I'll add your ack, if you agree with the new version of the patch.

Thanks,
Jason.

View attachment "ppc-dbg-traps.patch" of type "text/x-diff" (2474 bytes)