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

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

[K.Prasad]

[-- Attachment #1: fix_issues_hwbkpt_NEW_01 --]
[-- Type: text/plain, Size: 2679 bytes --]

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@linux.vnet.ibm.com>
---
 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);
 		/*
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;
 
 	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);
+
 	spin_unlock_bh(&hw_breakpoint_lock);
 }
 EXPORT_SYMBOL_GPL(unregister_user_hw_breakpoint);

[Patch 2/2] Fix the style issue seen in ksym tracer ftrace plugin

[K.Prasad]

[-- Attachment #1: fix_style_issue_ftrace_NEW_02 --]
[-- Type: text/plain, Size: 1295 bytes --]

Fix the style issue seen in ksym tracer ftrace plugin

Move the extern 'process_new_ksym_entry' declaration from
trace_selftest.c to trace.h file due to a reported style issue.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 kernel/trace/trace.h          |    1 +
 kernel/trace/trace_selftest.c |    1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-tip.hbkpt/kernel/trace/trace.h
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace.h
+++ linux-2.6-tip.hbkpt/kernel/trace/trace.h
@@ -213,6 +213,7 @@ struct syscall_trace_exit {
 };
 
 #define KSYM_SELFTEST_ENTRY "ksym_selftest_dummy"
+extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
 
 struct trace_ksym {
 	struct trace_entry	ent;
Index: linux-2.6-tip.hbkpt/kernel/trace/trace_selftest.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_selftest.c
+++ linux-2.6-tip.hbkpt/kernel/trace/trace_selftest.c
@@ -810,7 +810,6 @@ trace_selftest_startup_hw_branches(struc
 #endif /* CONFIG_HW_BRANCH_TRACER */
 
 #ifdef CONFIG_KSYM_TRACER
-extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
 static int ksym_selftest_dummy;
 
 int

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

[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@linux.vnet.ibm.com>
> ---
>  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

Re: [Patch 2/2] Fix the style issue seen in ksym tracer ftrace plugin

[Frederic Weisbecker]

On Tue, May 19, 2009 at 09:55:58PM +0530, K.Prasad wrote:
> Fix the style issue seen in ksym tracer ftrace plugin
> 
> Move the extern 'process_new_ksym_entry' declaration from
> trace_selftest.c to trace.h file due to a reported style issue.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>


> ---
>  kernel/trace/trace.h          |    1 +
>  kernel/trace/trace_selftest.c |    1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace.h
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace.h
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace.h
> @@ -213,6 +213,7 @@ struct syscall_trace_exit {
>  };
>  
>  #define KSYM_SELFTEST_ENTRY "ksym_selftest_dummy"
> +extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
>  
>  struct trace_ksym {
>  	struct trace_entry	ent;
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace_selftest.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_selftest.c
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace_selftest.c
> @@ -810,7 +810,6 @@ trace_selftest_startup_hw_branches(struc
>  #endif /* CONFIG_HW_BRANCH_TRACER */
>  
>  #ifdef CONFIG_KSYM_TRACER
> -extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
>  static int ksym_selftest_dummy;
>  
>  int
> 

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

[K.Prasad]

On Tue, May 19, 2009 at 02:09:12PM -0400, Alan Stern wrote:
> 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@linux.vnet.ibm.com>
> > ---
> >  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:
> 

Agreed, we want to return NOTIFY_DONE in the above two cases and the
patch does the same.

If there are any more breakpoints (basically more trap bits in DR6 set)
that require handling, the "continue" statement allows it to be done.

If the non-breakpoint bits were set in DR6, then "rc" value is
over-written in hw_breakpoint_handler() below the "if (!bp)" check.

	if (dr6 & (~DR_TRAP_BITS))
		rc = NOTIFY_DONE;

The issue in the original hw_breakpoint_handler() is the return of
NOTIFY_DONE during lazy debug register switching or mismatched
this_hbp_kernel[], that may cause SIGTRAP signal for user-space
in do_debug() function.

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

This change will unconditionally return NOTIFY_DONE even if exception
was triggered due to a single kernel-space HW breakpoint.

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

I've added a descriptive comment. Let me know if it can be improved.

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

Changed.

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

Ok. I have changed it.

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

Please find the updated patch below and kindly let me know your
comments on the same.


Improvements and minor fixes to HW Breakpoint interface

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@linux.vnet.ibm.com>
---
 arch/x86/kernel/hw_breakpoint.c |   17 ++++++++++++++++-
 arch/x86/kernel/ptrace.c        |    4 +---
 kernel/hw_breakpoint.c          |   19 ++++++++++++++-----
 3 files changed, 31 insertions(+), 9 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
@@ -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
+ * 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);
 		/*
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,22 @@ void unregister_user_hw_breakpoint(struc
 						struct hw_breakpoint *bp)
 {
 	struct thread_struct *thread = &(tsk->thread);
-	int i;
+	int i, pos = -1, hbp_counter = 0;
 
 	spin_lock_bh(&hw_breakpoint_lock);
 	for (i = 0; i < hbp_kernel_pos; i++) {
-		if (bp == thread->hbp[i]) {
-			__unregister_user_hw_breakpoint(i, tsk);
-			break;
-		}
+		if (thread->hbp[i])
+			hbp_counter++;
+		if (bp == thread->hbp[i])
+			pos = i;
 	}
+	if (pos >= 0) {
+		__unregister_user_hw_breakpoint(pos, tsk);
+		hbp_counter--;
+	}
+	if (!hbp_counter)
+		clear_tsk_thread_flag(tsk, TIF_DEBUG);
+
 	spin_unlock_bh(&hw_breakpoint_lock);
 }
 EXPORT_SYMBOL_GPL(unregister_user_hw_breakpoint);

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

[Alan Stern]

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

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

[K.Prasad]

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

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

[Alan Stern]

On Wed, 20 May 2009, K.Prasad wrote:

> 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;
> }

This looks okay.

Alan Stern