[PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[Frederic Weisbecker]

Before we had a generic breakpoint API, ptrace was accepting
breakpoints on NULL address in x86. The new API refuse them,
without given strong reasons. We need to follow the previous
behaviour as some userspace apps like Wine need such NULL
breakpoints to ensure old emulated software protections
are still working.

This fixes a 2.6.32 - 2.6.33-x ptrace regression.

Reported-by: Michael Stefaniuc <mstefani@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: K.Prasad <prasad@linux.vnet.ibm.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Maneesh Soni <maneesh@linux.vnet.ibm.com>
Cc: Alexandre Julliard <julliard@winehq.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Maciej Rutecki <maciej.rutecki@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c |   30 +++++++-----------------------
 1 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 05d5fec..bb6006e 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -212,25 +212,6 @@ static int arch_check_va_in_kernelspace(unsigned long va, u8 hbp_len)
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
 
-/*
- * Store a breakpoint's encoded address, length, and type.
- */
-static int arch_store_info(struct perf_event *bp)
-{
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	/*
-	 * For kernel-addresses, either the address or symbol name can be
-	 * specified.
-	 */
-	if (info->name)
-		info->address = (unsigned long)
-				kallsyms_lookup_name(info->name);
-	if (info->address)
-		return 0;
-
-	return -EINVAL;
-}
-
 int arch_bp_generic_fields(int x86_len, int x86_type,
 			   int *gen_len, int *gen_type)
 {
@@ -362,10 +343,13 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp,
 		return ret;
 	}
 
-	ret = arch_store_info(bp);
-
-	if (ret < 0)
-		return ret;
+	/*
+	 * For kernel-addresses, either the address or symbol name can be
+	 * specified.
+	 */
+	if (info->name)
+		info->address = (unsigned long)
+				kallsyms_lookup_name(info->name);
 	/*
 	 * Check that the low-order bits of the address are appropriate
 	 * for the alignment implied by len.
-- 
1.6.2.3

Re: [PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[Roland McGrath]

Setting a watchpoint on address 0 seems like an entirely reasonable thing
to do.  This change looks right to me.

Thanks,
Roland

Re: [PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[James Kosin]

On 2/18/2010 4:20 PM, Roland McGrath wrote:
> Setting a watchpoint on address 0 seems like an entirely reasonable thing
> to do.  This change looks right to me.
> 
> Thanks,
> Roland

Okay with me.  Most platforms use address 0x0000 for the RESET vector.
Nice to catch; only problem was if it was a true RESET the information
is gone anyway.
If not a true RESET, the caller tried to call a function at 0x0000 which
then really isn't allowed under most circumstances.

James

Re: [PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[K.Prasad]

On Thu, Feb 18, 2010 at 07:00:00PM +0100, Frederic Weisbecker wrote:
> Before we had a generic breakpoint API, ptrace was accepting
> breakpoints on NULL address in x86. The new API refuse them,
> without given strong reasons. We need to follow the previous
> behaviour as some userspace apps like Wine need such NULL
> breakpoints to ensure old emulated software protections
> are still working.
> 
> This fixes a 2.6.32 - 2.6.33-x ptrace regression.
> 
> Reported-by: Michael Stefaniuc <mstefani@redhat.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: K.Prasad <prasad@linux.vnet.ibm.com>

Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>

> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Maneesh Soni <maneesh@linux.vnet.ibm.com>
> Cc: Alexandre Julliard <julliard@winehq.org>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Maciej Rutecki <maciej.rutecki@gmail.com>
> ---
>  arch/x86/kernel/hw_breakpoint.c |   30 +++++++-----------------------
>  1 files changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 05d5fec..bb6006e 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -212,25 +212,6 @@ static int arch_check_va_in_kernelspace(unsigned long va, u8 hbp_len)
>  	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
>  }
> 
> -/*
> - * Store a breakpoint's encoded address, length, and type.
> - */
> -static int arch_store_info(struct perf_event *bp)
> -{
> -	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	/*
> -	 * For kernel-addresses, either the address or symbol name can be
> -	 * specified.
> -	 */
> -	if (info->name)
> -		info->address = (unsigned long)
> -				kallsyms_lookup_name(info->name);
> -	if (info->address)
> -		return 0;
> -
> -	return -EINVAL;
> -}
> -
>  int arch_bp_generic_fields(int x86_len, int x86_type,
>  			   int *gen_len, int *gen_type)
>  {
> @@ -362,10 +343,13 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp,
>  		return ret;
>  	}
> 
> -	ret = arch_store_info(bp);
> -
> -	if (ret < 0)
> -		return ret;
> +	/*
> +	 * For kernel-addresses, either the address or symbol name can be
> +	 * specified.
> +	 */
> +	if (info->name)
> +		info->address = (unsigned long)
> +				kallsyms_lookup_name(info->name);
>  	/*
>  	 * Check that the low-order bits of the address are appropriate
>  	 * for the alignment implied by len.
> -- 
> 1.6.2.3
> 

Re: [PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[Frederic Weisbecker]

On Thu, Feb 18, 2010 at 01:16:59PM -0800, Roland McGrath wrote:
> Setting a watchpoint on address 0 seems like an entirely reasonable thing
> to do.  This change looks right to me.
> 
> Thanks,
> Roland

I'm adding you ack then. Thanks.

[PATCH 1/2] hw-breakpoints: Accept breakpoints on NULL address

[Frederic Weisbecker]

Before we had a generic breakpoint API, ptrace was accepting
breakpoints on NULL address in x86. The new API refuse them,
without given strong reasons. We need to follow the previous
behaviour as some userspace apps like Wine need such NULL
breakpoints to ensure old emulated software protections
are still working.

This fixes a 2.6.32 - 2.6.33-x ptrace regression.

Reported-and-tested-by: Michael Stefaniuc <mstefani@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>
Acked-by: Roland McGrath <roland@redhat.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Maneesh Soni <maneesh@linux.vnet.ibm.com>
Cc: Alexandre Julliard <julliard@winehq.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Maciej Rutecki <maciej.rutecki@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c |   30 +++++++-----------------------
 1 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 05d5fec..bb6006e 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -212,25 +212,6 @@ static int arch_check_va_in_kernelspace(unsigned long va, u8 hbp_len)
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
 
-/*
- * Store a breakpoint's encoded address, length, and type.
- */
-static int arch_store_info(struct perf_event *bp)
-{
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	/*
-	 * For kernel-addresses, either the address or symbol name can be
-	 * specified.
-	 */
-	if (info->name)
-		info->address = (unsigned long)
-				kallsyms_lookup_name(info->name);
-	if (info->address)
-		return 0;
-
-	return -EINVAL;
-}
-
 int arch_bp_generic_fields(int x86_len, int x86_type,
 			   int *gen_len, int *gen_type)
 {
@@ -362,10 +343,13 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp,
 		return ret;
 	}
 
-	ret = arch_store_info(bp);
-
-	if (ret < 0)
-		return ret;
+	/*
+	 * For kernel-addresses, either the address or symbol name can be
+	 * specified.
+	 */
+	if (info->name)
+		info->address = (unsigned long)
+				kallsyms_lookup_name(info->name);
 	/*
 	 * Check that the low-order bits of the address are appropriate
 	 * for the alignment implied by len.
-- 
1.6.2.3