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

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 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[K.Prasad]

On Fri, Feb 19, 2010 at 06:41:53PM +0100, Frederic Weisbecker wrote:
> On Fri, Feb 19, 2010 at 02:28:31PM +0530, K.Prasad wrote:
> > On Thu, Feb 18, 2010 at 07:00:01PM +0100, Frederic Weisbecker wrote:
> > 
> > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > index 017d937..0c1033d 100644
> > > --- a/arch/x86/kernel/ptrace.c
> > > +++ b/arch/x86/kernel/ptrace.c
> > > @@ -702,7 +702,7 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
> > >  	} else if (n == 6) {
> > >  		val = thread->debugreg6;
> > >  	 } else if (n == 7) {
> > > -		val = ptrace_get_dr7(thread->ptrace_bps);
> > > +		val = thread->ptrace_dr7;
> > 
> > Some more comments that I missed out in the previous mail...
> > 
> > Shouldn't ptrace_get_dr7() function be entirely removed now, given that
> > its only caller no longer invokes it?
> > 
> > >  	}
> > >  	return val;
> > >  }
> > > @@ -778,8 +778,11 @@ int ptrace_set_debugreg(struct task_struct *tsk, int n, unsigned long val)
> > >  			return rc;
> > >  	}
> > >  	/* All that's left is DR7 */
> > > -	if (n == 7)
> > > +	if (n == 7) {
> > >  		rc = ptrace_write_dr7(tsk, val);
> > 
> > And ptrace_write_dr7() should be made to populate thread->ptrace_dr7 if
> > it is going to return a success.
> > 
> > > +		if (!rc)
> > > +			thread->ptrace_dr7 = val;
> > > +	}
> > > 
> > >  ret_path:
> > >  	return rc;
> > > -- 
> > > 1.6.2.3
> > > 
> > 
> > Thanks,
> > K.Prasad
> > 
> 
> 
> This is urgent so I'm pushing the two patches right away as they fix the
> regression and are not intrusive.
> 
> Let's further improve that later, for a merge window, ok?
> 
> Thanks.
>

Sure, please go ahead with the patch.

Thanks,
K.Prasad
 

[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

Re: Regression in ptrace (Wine) starting with 2.6.33-rc1

[Michael Stefaniuc]

On 02/14/2010 09:41 PM, Frederic Weisbecker wrote:
> On Sun, Feb 14, 2010 at 09:13:06PM +0100, Michael Stefaniuc wrote:
>> Although Wine will map address 0x0 for DOS programs that isn't the
>> reason for those tests. Wine has to support games that come with
>> pointless copy protection schemes that employ that technique.
> Ah, which kind of protection?
No clue as I'm not into games. But the wiki has a page for that 
http://wiki.winehq.org/CopyProtection


>> Cool, thanks!
>> Any chance to get that fix into 2.6.33?
> Yeah.
>
> Could you please test the following patch on top of
> 2.6.33-rc9 ?
It is an improvement as I don't get an -EINVAL now but the data in DR7
is not what was written there and the test fails with:
exception.c:612: Test failed: failed to set debugregister 7 to 0x155, 
got 2aa

The corresponding ptrace calls for that test are:
ptrace(PTRACE_ATTACH, 3368, 0, 0)       = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 28, 
0x42424242) = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 32, 0) = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 36, 0) = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 40, 0) = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 52, 0) = 0
ptrace(PTRACE_POKEUSER, 3368, offsetof(struct user, i387) + 56, 0x155) = 0
ptrace(PTRACE_DETACH, 3368, 0x1, SIG_0) = 0
ptrace(PTRACE_ATTACH, 3368, 0, 0)       = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 28, 
[0xfffffffc42424242]) = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 32, 
[0xfffffffd00000000]) = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 36, 
[0xfffffffe00000000]) = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 40, 
[0xffffffff00000000]) = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 52, 
[0x200000000]) = 0
ptrace(PTRACE_PEEKUSER, 3368, offsetof(struct user, i387) + 56, 
[0x3000002aa]) = 0
ptrace(PTRACE_DETACH, 3368, 0x1, SIG_0) = 0

> I'm trying to build wine but it fails because my libx11 is
> incorrect for the linking (probably because I don't have a x86-32
> version of libx11.so):
The easiest to bootstrap the build environment is to use the package
management of the distribution, e.g. yum-builddep wine on Fedora.  But
there are also howto's for other distributions on
http://wiki.winehq.org/WineOn64bit

> 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.
>
>
>
>> I cannot test that as the corresponding test is directly affected by
>> this ABI change.
>
>
> Sure, let's fix the first problem to begin.
That regression isn't there anymore; I had seen it when the regression
search brought me to 66cb591. Now all other tests in ntdll/exception.c
pass just fine.

thanks
bye
	michael

[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

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

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
>