linux-kernel - Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag topassDIE

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 for Android: free password hash cracker in your pocket

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

Date:	Fri, 23 Jul 2010 10:49:20 -0500
From:	Jason Wessel <jason.wessel@...driver.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
CC:	"Deng, Dongdong" <dongdong.deng@...driver.com>,
	will.deacon@....com, lethal@...ux-sh.org,
	mahesh@...ux.vnet.ibm.com, prasad@...ux.vnet.ibm.com,
	benh@...nel.crashing.org, paulus@...ba.org, mingo@...e.hu,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag topassDIE_DEBUG
 notification

On 07/23/2010 09:07 AM, Frederic Weisbecker wrote:
> On Fri, Jul 23, 2010 at 08:19:54AM -0500, Jason Wessel wrote:
>   
>> On 07/23/2010 08:04 AM, Frederic Weisbecker wrote:
>>     
>> The patch may or may not be the right way to solve the problem.   It is
>> worth noting that early breakpoints are handled separately with a direct
>> writes to the debug registers so this API does not apply.
>>     
>
>
>
> But you still need to handle them on the debug exception, right?
>
>
>   

Yes, but at that point kgdb is first in line for the notifier so it
works out of the box.

>  
>   
>> This patch effectively causes the events to get passed to the normal
>> handlers.
>>
>> The source of the original problem (which was merged in 2.6.35) is
>> commit: 018cbffe6819f6f8db20a0a3acd9bab9bfd667e4 -     Merge commit
>> 'v2.6.33' into perf/core
>>
>> Specifically this line right here:
>> @@@ -502,6 -486,8 +486,6 @@@ static int __kprobes hw_breakpoint_hand
>>           rcu_read_lock();
>>  
>>           bp = per_cpu(bp_per_reg[i], cpu);
>>  -        if (bp)
>>  -            rc = NOTIFY_DONE;
>>
>> Because the NOTIFY_DONE is never set, a default value of NOTIFY_STOP is
>> passed at the end and kgdb never gets to see the break point even that
>> was never intended for the perf handler in the first place.
>>
>> It is not as easy of course to just revert this patch because it changed
>> other logic.
>>
>> Jason.
>>     
>
>
>
> Right.
>
> Actually NOTIFY_DONE is returned when there is more work to do: handling
> another exception than breakpoint, or sending a signal. Otherwise yeah,
> we return NOTIFY_STOP as we assume there is more work to do.
>
>   

For this specific case the hw_breakpoint handler simply consumed a
breakpoint which was not intended for it.

> So the following alternatives appear to me:
>
> - Moving the breakpoint exception handling into the
>   struct perf_event:overflow_handler. In fact I can't find the breakpoint
>   handling in kgdb.c
>
>   

It is in the generic die notification handler for kgdb (looking at
2.6.35-rc6)

arch/x86/kernel/kgdb.c

    516 static int __kgdb_notify(struct die_args *args, unsigned long cmd)
...
    551         case DIE_DEBUG:
    552                 if (atomic_read(&kgdb_cpu_doing_single_step) !=
-1) {
    553                         if (user_mode(regs))
    554                                 return single_step_cont(regs, args);
    555                         break;
    556                 } else if (test_thread_flag(TIF_SINGLESTEP))
    557                         /* This means a user thread is single
stepping
    558                          * a system call which should be ignored
    559                          */
    560                         return NOTIFY_DONE;
    561                 /* fall through */


> - Have a higher priority in kgdb notifier (which means decreasing the one
>   of hw_breakpoint.c)
>   

kgdb had always been last in line in arch/x86/kernel/kgdb.c:

    608 static struct notifier_block kgdb_notifier = {
    609         .notifier_call  = kgdb_notify,
    610
    611         /*
    612          * Lowest-prio notifier priority, we want to be notified
last:
    613          */
    614         .priority       = -INT_MAX,
    615 };


> - Always returning NOTIFY_DONE from the breakpoint path.
>
>   

Without some further investigation, I am not sure what this will do.  We
don't want to make things worse of course.  Because kgdb uses the
request hw_breakpoint api to request slot reservation having an
attribute to say don't do anything to this HW breakpoint is certainly
one way to fix it.

> Is this a regression BTW?
>
>   

Absolutely this is a regression.  No change was made in kgdb related to
this and the kgdb HW breakpoint regression tests (which come with the
kernel) stopped working and bisect to the commit I mentioned.


Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/