Re: [Patch 1/2] Improvements and minor fixes to HW Breakpoint interface

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Wed, May 20, 2009 at 10:04:09AM -0400, Alan Stern wrote:
> On Wed, 20 May 2009, K.Prasad wrote:
> 
> > --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/hw_breakpoint.c
> > +++ linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
> > @@ -296,6 +296,19 @@ void arch_flush_thread_hw_breakpoint(str
> >  
> >  /*
> >   * Handle debug exception notifications.
> > + *
> > + * Return value is either NOTIFY_STOP or NOTIFY_DONE as explained below.
> > + *
> > + * NOTIFY_DONE returned if one of the following conditions is true.
> > + * i) When the causative address is from user-space and the exception
> > + * is a valid one i.e. not triggered a result of lazy debug register
> --------------------^----^  missing ',' characters
> ---------------------------------------^ missing "as"
> 
> > + * switching
> > + * ii) When there are more bits than trap<n> set in DR6 register (such
> > + * as BD, BS or BT) indicating that more than one debug condition is
> > + * met and requires some more action in do_debug().
> > + *
> > + * NOTIFY_STOP returned for all other cases
> > + *
> >   */
> >  int __kprobes hw_breakpoint_handler(struct die_args *args)
> >  {
> > @@ -346,8 +359,10 @@ int __kprobes hw_breakpoint_handler(stru
> >  		 * or due to the delay between updates of hbp_kernel_pos
> >  		 * and this_hbp_kernel.
> >  		 */
> > -		if (!bp)
> > +		if (!bp) {
> > +			rc = NOTIFY_STOP;
> >  			continue;
> > +		}
> >  
> >  		(bp->triggered)(bp, args->regs);
> 
> This is the same mistake as before.  If rc had previously been set to 
> NOTIFY_DONE then you will unconditionally change it back to 
> NOTIFY_STOP.  You should make the code do what the comment says, set rc
> to NOTIFY_DONE when a non-NULL user breakpoint occurs:
> 
> 		if (i >= hbp_kernel_pos) {
> 			bp = per_cpu(this_hbp_kernel[i], cpu);
> 		} else {
> 			bp = current->thread.hbp[i];
> 			if (bp)
> 				rc = NOTIFY_DONE;
> 		}
> 		/*
> 		 * bp can be NULL due to lazy debug register switching
> 		 * or due to the delay between updates of hbp_kernel_pos
> 		 * and this_hbp_kernel.
> 		 */
> 		if (!bp)
> 			continue;
> 
> 		(bp->triggered)(bp, args->regs);
> 
> 
> The rest of the changes are okay.
> 
> Alan Stern
>

Thanks. I am pasting the hw_breakpoint_handler() function completely
below (as opposed to patch form which may be difficult to follow). Upon
your consent with the changes, I intend to send a consolidated patchset
to Ingo (merging the patches over hw_breakpoint.c) for inclusion in -tip
tree.

I have also moved the "(*dr6_p) &= ~(DR_TRAP0 << i)" before the "if (!bp)"
check.

/*
 * Handle debug exception notifications.
 *
 * Return value is either NOTIFY_STOP or NOTIFY_DONE as explained below.
 *
 * NOTIFY_DONE returned if one of the following conditions is true.
 * i) When the causative address is from user-space and the exception
 * is a valid one, i.e. not triggered as a result of lazy debug register
 * switching
 * ii) When there are more bits than trap<n> set in DR6 register (such
 * as BD, BS or BT) indicating that more than one debug condition is
 * met and requires some more action in do_debug().
 *
 * NOTIFY_STOP returned for all other cases
 *
 */
int __kprobes hw_breakpoint_handler(struct die_args *args)
{
	int i, cpu, rc = NOTIFY_STOP;
	struct hw_breakpoint *bp;
	unsigned long dr7, dr6;
	unsigned long *dr6_p;

	/* The DR6 value is pointed by args->err */
	dr6_p = (unsigned long *)ERR_PTR(args->err);
	dr6 = *dr6_p;

	/* Do an early return if no trap bits are set in DR6 */
	if ((dr6 & DR_TRAP_BITS) == 0)
		return NOTIFY_DONE;

	/* Lazy debug register switching */
	if (!test_tsk_thread_flag(current, TIF_DEBUG))
		arch_uninstall_thread_hw_breakpoint();

	get_debugreg(dr7, 7);
	/* Disable breakpoints during exception handling */
	set_debugreg(0UL, 7);
	/*
	 * Assert that local interrupts are disabled
	 * Reset the DRn bits in the virtualized register value.
	 * The ptrace trigger routine will add in whatever is needed.
	 */
	current->thread.debugreg6 &= ~DR_TRAP_BITS;
	cpu = get_cpu();

	/* Handle all the breakpoints that were triggered */
	for (i = 0; i < HBP_NUM; ++i) {
		if (likely(!(dr6 & (DR_TRAP0 << i))))
			continue;
		/*
		 * Find the corresponding hw_breakpoint structure and
		 * invoke its triggered callback.
		 */
		if (i >= hbp_kernel_pos)
			bp = per_cpu(this_hbp_kernel[i], cpu);
		else {
			bp = current->thread.hbp[i];
			if (bp)
				rc = NOTIFY_DONE;
		}

		/*
		 * Reset the 'i'th TRAP bit in dr6 to denote completion of
		 * exception handling
		 */
		(*dr6_p) &= ~(DR_TRAP0 << i);
		/*
		 * bp can be NULL due to lazy debug register switching
		 * or due to the delay between updates of hbp_kernel_pos
		 * and this_hbp_kernel.
		 */
		if (!bp)
			continue;

		(bp->triggered)(bp, args->regs);
	}
	if (dr6 & (~DR_TRAP_BITS))
		rc = NOTIFY_DONE;

	set_debugreg(dr7, 7);
	put_cpu_no_resched();
	return rc;
}

Thanks,
K.Prasad