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

[Alan Stern]

On Tue, 19 May 2009, K.Prasad wrote:

> This patch brings a couple of changes to the HW Breakpoint infrastructure:
> 
> - Set/clear TIF_DEBUG task flag in <un>register_user_hw_breakpoint()
>   instead of being done by users of this interface (such as ptrace).
> 
> - Modify return code of hw_breakpoint_handler() to NOTIFY_STOP if
>   triggered due to lazy debug register switching.
> 
> Signed-off-by: K.Prasad <prasad@xxxxxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/hw_breakpoint.c |    4 +++-
>  arch/x86/kernel/ptrace.c        |    4 +---
>  kernel/hw_breakpoint.c          |   16 +++++++++++++---
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/hw_breakpoint.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
> @@ -346,8 +346,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 doesn't look right.  You want to return NOTIFY_DONE if any
breakpoints were triggered or any non-breakpoint bits were set in DR6,
right?  Otherwise return NOTIFY_STOP.  So this code should be:

		if (!bp)
			continue;

		rc = NOTIFY_DONE;
		(bp->triggered)(bp, args->regs);

and take out the "rc = NOTIFY_DONE" line in the i < hbp_kernel_pos 
case earlier.

It also would be a good idea to add a comment near the start of the
function explaining the return value (just paraphrase what I wrote
above).


> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/ptrace.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/ptrace.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/ptrace.c
> @@ -530,9 +530,7 @@ restore:
>  				bp->info.len = len;
>  				bp->info.type = type;
>  				rc = register_user_hw_breakpoint(tsk, bp);
> -				if (!rc)
> -					set_tsk_thread_flag(tsk, TIF_DEBUG);
> -				else
> +				if (rc)
>  					kfree(bp);
>  			}
>  		} else
> Index: linux-2.6-tip.hbkpt/kernel/hw_breakpoint.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/hw_breakpoint.c
> +++ linux-2.6-tip.hbkpt/kernel/hw_breakpoint.c
> @@ -233,6 +233,8 @@ int register_user_hw_breakpoint(struct t
>  			break;
>  		}
>  	}
> +	if (!rc)
> +		set_tsk_thread_flag(tsk, TIF_DEBUG);
>  
>  	spin_unlock_bh(&hw_breakpoint_lock);
>  	return rc;
> @@ -272,15 +274,23 @@ void unregister_user_hw_breakpoint(struc
>  						struct hw_breakpoint *bp)
>  {
>  	struct thread_struct *thread = &(tsk->thread);
> -	int i;
> +	int i, pos = -1, clear_tsk_debug_counter = 0;

I think "hbp_counter" would be a better name.  It's up to you.

>  
>  	spin_lock_bh(&hw_breakpoint_lock);
>  	for (i = 0; i < hbp_kernel_pos; i++) {
> +		if (thread->hbp[i])
> +			clear_tsk_debug_counter++;
>  		if (bp == thread->hbp[i]) {
> -			__unregister_user_hw_breakpoint(i, tsk);
> -			break;
> +			clear_tsk_debug_counter--;
> +			pos = i;
>  		}
>  	}
> +	if (pos >= 0)
> +		__unregister_user_hw_breakpoint(pos, tsk);
> +
> +	if (!clear_tsk_debug_counter)
> +		clear_tsk_thread_flag(tsk, TIF_DEBUG);

This logic could be rearranged a little:

	for (i = 0; i < hbp_kernel_pos; i++) {
		if (thread->hbp[i])
			clear_tsk_debug_counter++;
		if (bp == thread->hbp[i])
			pos = i;
	}
	if (pos >= 0) {
		__unregister_user_hw_breakpoint(pos, tsk);
		clear_tsk_debug_counter--;
	}
	if (!clear_tsk_debug_counter)
		clear_tsk_thread_flag(tsk, TIF_DEBUG);

Not a big deal; this way is a little more robust in case two 
thread->hbp[] entries somehow contain the same pointer.

> +
>  	spin_unlock_bh(&hw_breakpoint_lock);
>  }
>  EXPORT_SYMBOL_GPL(unregister_user_hw_breakpoint);

Alan Stern

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