Regression in ptrace (Wine) starting with 2.6.33-rc1

Regression in ptrace (Wine) starting with 2.6.33-rc1

[Michael Stefaniuc]

Hello!

2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
registers. This is visible in the Wine ntdll exception tests failing on
2.6.33-rcX while they work just fine in 2.6.32.

A regression test resulted in:
72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
Author: K.Prasad <prasad@linux.vnet.ibm.com>
Date:   Mon Jun 1 23:45:48 2009 +0530

    hw-breakpoints: modify Ptrace routines to access breakpoint registers

    This patch modifies the ptrace code to use the new wrapper routines
around
the
    debug/breakpoint registers.

    [ Impact: adapt x86 ptrace to the new breakpoint Api ]

    Original-patch-by: Alan Stern <stern@rowland.harvard.edu>
    Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
    Signed-off-by: Maneesh Soni <maneesh@linux.vnet.ibm.com>
    Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

:040000 040000 f72ff4760c3fa1dffcd72494e77bee2c76039505
b60d5fe2088ff635568e800d5759a0b373b5e439 M      arch


The first ntdll exception test in test_exceptions()
http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ntdll/tests/exception.c;h=9149b6961764dec31a0af5cd3b93ab3072703dbb;hb=312e4f6b235a468f8bf764101a5b97cf34dd4143#l594
  run_exception_test(dreg_handler, NULL, &segfault_code,
sizeof(segfault_code),
0);
produces (make exception.ok) the output:
  err:seh:setup_exception_record stack overflow 932 bytes in thread 0009 eip
7bc3c97f esp 00240f8c stack 0x240000-0x241000-0x340000
The stack overflow is detected by the ntdll internal function
setup_exception_record()
http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ntdll/signal_i386.c;h=4eccb61954c43d75144575411313d59405decfc3;hb=312e4f6b235a468f8bf764101a5b97cf34dd4143#l1495
which aborts the thread.
The problem happens on both i386 (Intel Atom CPU) as well as on x86_64
(Intel Q9450); the stack overflow bytes differ though but are always the
same for each box.

All the ntdll exception tests run just fine with 2.6.32 and older
kernels. For a summary of the ntdll exception tests please see
http://test.winehq.org/data/tests/ntdll:exception.html in the Wine
column.

I have opened also http://bugzilla.kernel.org/show_bug.cgi?id=15273 for
this.

thanks
bye
    michael

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

[Frederic Weisbecker]

On Thu, Feb 11, 2010 at 05:33:13PM +0100, Michael Stefaniuc wrote:
> Hello!
> 
> 2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
> registers. This is visible in the Wine ntdll exception tests failing on
> 2.6.33-rcX while they work just fine in 2.6.32.
> 
> A regression test resulted in:
> 72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
> commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
> Author: K.Prasad <prasad@linux.vnet.ibm.com>
> Date:   Mon Jun 1 23:45:48 2009 +0530
> 
>     hw-breakpoints: modify Ptrace routines to access breakpoint registers
> 
>     This patch modifies the ptrace code to use the new wrapper routines
> around
> the
>     debug/breakpoint registers.
> 
>     [ Impact: adapt x86 ptrace to the new breakpoint Api ]
> 
>     Original-patch-by: Alan Stern <stern@rowland.harvard.edu>
>     Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
>     Signed-off-by: Maneesh Soni <maneesh@linux.vnet.ibm.com>
>     Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
>     Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> :040000 040000 f72ff4760c3fa1dffcd72494e77bee2c76039505
> b60d5fe2088ff635568e800d5759a0b373b5e439 M      arch
> 
> 
> The first ntdll exception test in test_exceptions()
> http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ntdll/tests/exception.c;h=9149b6961764dec31a0af5cd3b93ab3072703dbb;hb=312e4f6b235a468f8bf764101a5b97cf34dd4143#l594
>   run_exception_test(dreg_handler, NULL, &segfault_code,
> sizeof(segfault_code),
> 0);
> produces (make exception.ok) the output:
>   err:seh:setup_exception_record stack overflow 932 bytes in thread 0009 eip
> 7bc3c97f esp 00240f8c stack 0x240000-0x241000-0x340000
> The stack overflow is detected by the ntdll internal function
> setup_exception_record()
> http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ntdll/signal_i386.c;h=4eccb61954c43d75144575411313d59405decfc3;hb=312e4f6b235a468f8bf764101a5b97cf34dd4143#l1495
> which aborts the thread.
> The problem happens on both i386 (Intel Atom CPU) as well as on x86_64
> (Intel Q9450); the stack overflow bytes differ though but are always the
> same for each box.
> 
> All the ntdll exception tests run just fine with 2.6.32 and older
> kernels. For a summary of the ntdll exception tests please see
> http://test.winehq.org/data/tests/ntdll:exception.html in the Wine
> column.
> 
> I have opened also http://bugzilla.kernel.org/show_bug.cgi?id=15273 for
> this.
> 
> thanks
> bye
>     michael


Thanks a lot for your report. Is there an easy way to reproduce
this?

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

[Michael Stefaniuc]

On 02/11/2010 07:22 PM, Frederic Weisbecker wrote:
> On Thu, Feb 11, 2010 at 05:33:13PM +0100, Michael Stefaniuc wrote:
>> 2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
>> registers. This is visible in the Wine ntdll exception tests failing on
>> 2.6.33-rcX while they work just fine in 2.6.32.
>>
>> A regression test resulted in:
>> 72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
>> commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
>> Author: K.Prasad<prasad@linux.vnet.ibm.com>
>> Date:   Mon Jun 1 23:45:48 2009 +0530
>>
>>      hw-breakpoints: modify Ptrace routines to access breakpoint registers
>>
>>      This patch modifies the ptrace code to use the new wrapper routines
>> around
>> the
>>      debug/breakpoint registers.
>>
>>      [ Impact: adapt x86 ptrace to the new breakpoint Api ]
>>
>>      Original-patch-by: Alan Stern<stern@rowland.harvard.edu>
>>      Signed-off-by: K.Prasad<prasad@linux.vnet.ibm.com>
>>      Signed-off-by: Maneesh Soni<maneesh@linux.vnet.ibm.com>
>>      Reviewed-by: Alan Stern<stern@rowland.harvard.edu>
>>      Signed-off-by: Frederic Weisbecker<fweisbec@gmail.com>
>>
>> :040000 040000 f72ff4760c3fa1dffcd72494e77bee2c76039505
>> b60d5fe2088ff635568e800d5759a0b373b5e439 M      arch
>>
>>

>> I have opened also http://bugzilla.kernel.org/show_bug.cgi?id=15273 for
>> this.

> Thanks a lot for your report. Is there an easy way to reproduce
> this?
Yes, the bug is 100% reproducible. Even the "stack overflow" bytes are
always constant on my two boxes: 932 bytes on my Atom and 1588 bytes on
my Q9450 with a x86_64 kernel.

Either grab wine-1.1.38 from
http://sourceforge.net/projects/wine/files/Source/ or from git
   git clone git://source.winehq.org/git/wine.git
configure
make
cd dlls/ntdll/tests/
make exception.ok

If you build on an x86_64 machine you'll need a pretty complete 32bit
setup too, but configure will let you know. If configure doesn't errors
out but produces warnings, those can be safely ignored. It means the
dependencies are optional and those aren't needed to reproduce this bug.

Oh, there might be an other regression in ptrace too; introduced by a
previous patch in this series. While bisecting i had a later test fail,
something along the lines of "expected 4 exceptions got 0", but the
tests completed. Now the stack corruption mask everything else in the
tests; e.g. comment out the first test and one of the next tests will go
into an infinite loop printing 3 Wine errors over and over again.

thanks
bye
         michael

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

[Frederic Weisbecker]

On Thu, Feb 11, 2010 at 08:49:48PM +0100, Michael Stefaniuc wrote:
> On 02/11/2010 07:22 PM, Frederic Weisbecker wrote:
>> On Thu, Feb 11, 2010 at 05:33:13PM +0100, Michael Stefaniuc wrote:
>>> 2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
>>> registers. This is visible in the Wine ntdll exception tests failing on
>>> 2.6.33-rcX while they work just fine in 2.6.32.
>>>
>>> A regression test resulted in:
>>> 72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
>>> commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
>>> Author: K.Prasad<prasad@linux.vnet.ibm.com>
>>> Date:   Mon Jun 1 23:45:48 2009 +0530
>>>
>>>      hw-breakpoints: modify Ptrace routines to access breakpoint registers
>>>
>>>      This patch modifies the ptrace code to use the new wrapper routines
>>> around
>>> the
>>>      debug/breakpoint registers.
>>>
>>>      [ Impact: adapt x86 ptrace to the new breakpoint Api ]
>>>
>>>      Original-patch-by: Alan Stern<stern@rowland.harvard.edu>
>>>      Signed-off-by: K.Prasad<prasad@linux.vnet.ibm.com>
>>>      Signed-off-by: Maneesh Soni<maneesh@linux.vnet.ibm.com>
>>>      Reviewed-by: Alan Stern<stern@rowland.harvard.edu>
>>>      Signed-off-by: Frederic Weisbecker<fweisbec@gmail.com>
>>>
>>> :040000 040000 f72ff4760c3fa1dffcd72494e77bee2c76039505
>>> b60d5fe2088ff635568e800d5759a0b373b5e439 M      arch
>>>
>>>
>
>>> I have opened also http://bugzilla.kernel.org/show_bug.cgi?id=15273 for
>>> this.
>
>> Thanks a lot for your report. Is there an easy way to reproduce
>> this?
> Yes, the bug is 100% reproducible. Even the "stack overflow" bytes are
> always constant on my two boxes: 932 bytes on my Atom and 1588 bytes on
> my Q9450 with a x86_64 kernel.
>
> Either grab wine-1.1.38 from
> http://sourceforge.net/projects/wine/files/Source/ or from git
>   git clone git://source.winehq.org/git/wine.git
> configure
> make
> cd dlls/ntdll/tests/
> make exception.ok
>
> If you build on an x86_64 machine you'll need a pretty complete 32bit
> setup too, but configure will let you know. If configure doesn't errors
> out but produces warnings, those can be safely ignored. It means the
> dependencies are optional and those aren't needed to reproduce this bug.



Ok, I'm going to test it.




> Oh, there might be an other regression in ptrace too; introduced by a
> previous patch in this series. While bisecting i had a later test fail,
> something along the lines of "expected 4 exceptions got 0", but the
> tests completed. Now the stack corruption mask everything else in the
> tests; e.g. comment out the first test and one of the next tests will go
> into an infinite loop printing 3 Wine errors over and over again.


Ok, will look at this too.

Thanks.

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

[K.Prasad]

On Thu, Feb 11, 2010 at 08:49:48PM +0100, Michael Stefaniuc wrote:
> On 02/11/2010 07:22 PM, Frederic Weisbecker wrote:
>> On Thu, Feb 11, 2010 at 05:33:13PM +0100, Michael Stefaniuc wrote:
>>> 2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
>>> registers. This is visible in the Wine ntdll exception tests failing on
>>> 2.6.33-rcX while they work just fine in 2.6.32.
>>>
>>> A regression test resulted in:
>>> 72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
>>> commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
>>> Author: K.Prasad<prasad@linux.vnet.ibm.com>
>>> Date:   Mon Jun 1 23:45:48 2009 +0530
>>>
>>>      hw-breakpoints: modify Ptrace routines to access breakpoint registers
>>>
>
>> Thanks a lot for your report. Is there an easy way to reproduce
>> this?
> Yes, the bug is 100% reproducible. Even the "stack overflow" bytes are
> always constant on my two boxes: 932 bytes on my Atom and 1588 bytes on
> my Q9450 with a x86_64 kernel.
>
> Either grab wine-1.1.38 from
> http://sourceforge.net/projects/wine/files/Source/ or from git
>   git clone git://source.winehq.org/git/wine.git
> configure
> make
> cd dlls/ntdll/tests/
> make exception.ok
>

Can you be more specific with details - such as what was the desired
action/return value of ptrace that your testcase wanted but did not
happen (after the patch applied)? What is the other regression that
you found as a result of another patch in the hw-breakpoint patch
series?

I am able to see a user-space stackdump upon a 'make exception.ok',
which isn't easy enough (atleast for me) to narrow down to the purported
ptrace defect.

> If you build on an x86_64 machine you'll need a pretty complete 32bit
> setup too, but configure will let you know. If configure doesn't errors
> out but produces warnings, those can be safely ignored. It means the
> dependencies are optional and those aren't needed to reproduce this bug.
>
> Oh, there might be an other regression in ptrace too; introduced by a
> previous patch in this series. While bisecting i had a later test fail,
> something along the lines of "expected 4 exceptions got 0", but the
> tests completed. Now the stack corruption mask everything else in the
> tests; e.g. comment out the first test and one of the next tests will go
> into an infinite loop printing 3 Wine errors over and over again.
>
> thanks
> bye
>         michael

Thanks,
K.Prasad

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

[Michael Stefaniuc]

On 02/13/2010 06:33 PM, K.Prasad wrote:
> On Thu, Feb 11, 2010 at 08:49:48PM +0100, Michael Stefaniuc wrote:
>> On 02/11/2010 07:22 PM, Frederic Weisbecker wrote:
>>> On Thu, Feb 11, 2010 at 05:33:13PM +0100, Michael Stefaniuc wrote:
>>>> 2.6.33-rc1 broke ptrace for Wine, specifically the setting of the debug
>>>> registers. This is visible in the Wine ntdll exception tests failing on
>>>> 2.6.33-rcX while they work just fine in 2.6.32.
>>>>
>>>> A regression test resulted in:
>>>> 72f674d203cd230426437cdcf7dd6f681dad8b0d is the first bad commit
>>>> commit 72f674d203cd230426437cdcf7dd6f681dad8b0d
>>>> Author: K.Prasad<prasad@linux.vnet.ibm.com>
>>>> Date:   Mon Jun 1 23:45:48 2009 +0530
>>>>
>>>>       hw-breakpoints: modify Ptrace routines to access breakpoint registers
>>>>
>>
>>> Thanks a lot for your report. Is there an easy way to reproduce
>>> this?
>> Yes, the bug is 100% reproducible. Even the "stack overflow" bytes are
>> always constant on my two boxes: 932 bytes on my Atom and 1588 bytes on
>> my Q9450 with a x86_64 kernel.
>>
>> Either grab wine-1.1.38 from
>> http://sourceforge.net/projects/wine/files/Source/ or from git
>>    git clone git://source.winehq.org/git/wine.git
>> configure
>> make
>> cd dlls/ntdll/tests/
>> make exception.ok
>>
>
> Can you be more specific with details - such as what was the desired
> action/return value of ptrace that your testcase wanted but did not
> happen (after the patch applied)? What is the other regression that
> you found as a result of another patch in the hw-breakpoint patch
> series?
>
> I am able to see a user-space stackdump upon a 'make exception.ok',
> which isn't easy enough (atleast for me) to narrow down to the purported
> ptrace defect.
Here is a discussion I had with the Wine maintainer on what that
specific test does exactly:
<julliard> puk: the test changes the debug regs in the context, which 
makes the server use ptrace to change the debug regs in the test process
<puk> cool
<puk> so i basically just do an strace on the server
<julliard> then it does a GetContext to verify that they have been set 
correctly
<julliard> yes all the ptrace calls are in the server
<puk> and capture what ptrace returns
<puk> let me guess GetContext uses ptrace too?
<julliard> yes
<julliard> if it even gets to that point, it sounded like it was 
crashing inside the exception handler

The wineserver is basically the "kernel space" in Wine.

Test setup:
-----------
# Start the wineserver and and attach to it
wineserver
strace -p $wineserver_pid >& strace.out
# Run the test
cd dlls/ntdll/tests/
make exception.ok

Results 2.6.33-rcX:
-------------------
ptrace(PTRACE_ATTACH, 18036, 0, 0)      = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg), 
0x42424242) = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 4, 0) = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 8, 0) = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 12, 
0) = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 24, 
0) = 0
ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 28, 
0x155) = -1 EINVAL (Invalid argument)

Results 2.6.32:
---------------
trace(PTRACE_ATTACH, 3077, 0, 0)       = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg), 
0x42424242) = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 4, 0) = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 8, 0) = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 12, 0) = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 24, 0) = 0
ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 28, 
0x155) = 0

So it looks like something in the setting of DR7 is broken or at least
changed behavior. The function in Wine that does those calls is
set_thread_context() from server/ptrace.c .

I'll try to see if I can reproduce the other regression; as it is hidden 
at the moment by this regression.

Thanks for looking at the problem.
bye
         michael

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

[Frederic Weisbecker]

On Sat, Feb 13, 2010 at 10:29:16PM +0100, Michael Stefaniuc wrote:
> Results 2.6.33-rcX:
> -------------------
> ptrace(PTRACE_ATTACH, 18036, 0, 0)      = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg),  
> 0x42424242) = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 4, 0) = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 8, 0) = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 12,  
> 0) = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 24,  
> 0) = 0
> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 28,  
> 0x155) = -1 EINVAL (Invalid argument)
>
> Results 2.6.32:
> ---------------
> trace(PTRACE_ATTACH, 3077, 0, 0)       = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg),  
> 0x42424242) = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 4, 0) = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 8, 0) = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 12, 0) = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 24, 0) = 0
> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 28,  
> 0x155) = 0


I see... So this is setting breakpoints on the address 0. The new code
rejects such breakpoints, but the previous one was accepting it.

The point of allowing breakpoints in NULL is discutable. It's not a bug,
neither is it a security hole I think (because if the ptrace breakpoint
triggers from the kernel, it's ignored), it's just pointless, unless
userland map things in 0.

But it's too late to debate this. If the previous code accepted it,
it's an ABI, and we have broken it.

I'm preparing a fix.



> So it looks like something in the setting of DR7 is broken or at least
> changed behavior. The function in Wine that does those calls is
> set_thread_context() from server/ptrace.c .
>
> I'll try to see if I can reproduce the other regression; as it is hidden  
> at the moment by this regression.


Ok.

Thanks.

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

[Michael Stefaniuc]

On 02/14/2010 06:15 PM, Frederic Weisbecker wrote:
> On Sat, Feb 13, 2010 at 10:29:16PM +0100, Michael Stefaniuc wrote:
>> Results 2.6.33-rcX:
>> -------------------
>> ptrace(PTRACE_ATTACH, 18036, 0, 0)      = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg),
>> 0x42424242) = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 4, 0) = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 8, 0) = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 12,
>> 0) = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 24,
>> 0) = 0
>> ptrace(PTRACE_POKEUSER, 18036, offsetof(struct user, u_debugreg) + 28,
>> 0x155) = -1 EINVAL (Invalid argument)
>>
>> Results 2.6.32:
>> ---------------
>> trace(PTRACE_ATTACH, 3077, 0, 0)       = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg),
>> 0x42424242) = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 4, 0) = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 8, 0) = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 12, 0) = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 24, 0) = 0
>> ptrace(PTRACE_POKEUSER, 3077, offsetof(struct user, u_debugreg) + 28,
>> 0x155) = 0
>
>
> I see... So this is setting breakpoints on the address 0. The new code
> rejects such breakpoints, but the previous one was accepting it.
>
> The point of allowing breakpoints in NULL is discutable. It's not a bug,
> neither is it a security hole I think (because if the ptrace breakpoint
> triggers from the kernel, it's ignored), it's just pointless, unless
> userland map things in 0.
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.

> But it's too late to debate this. If the previous code accepted it,
> it's an ABI, and we have broken it.
>
> I'm preparing a fix.
Cool, thanks!
Any chance to get that fix into 2.6.33?

>> So it looks like something in the setting of DR7 is broken or at least
>> changed behavior. The function in Wine that does those calls is
>> set_thread_context() from server/ptrace.c .
>>
>> I'll try to see if I can reproduce the other regression; as it is hidden
>> at the moment by this regression.
> Ok.
I cannot test that as the corresponding test is directly affected by 
this ABI change.

bye
	michael

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

[Frederic Weisbecker]

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?



> 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 ?
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):

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.

Thanks!

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

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

[K.Prasad]

On Mon, Feb 15, 2010 at 12:05:16AM +0100, Michael Stefaniuc wrote:
> 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
>

Okay, so this 0x155 written onto ptrace got converted into 0x2AA -
basically all requests to 'locally' enable breakpoints in DR0-DR3 (bits
0, 2, 4 and 6 of DR7) was converted into a request to 'globally' enable
(bits 1, 3, 5 and 7) breakpoints.

'Local' breakpoints - here would mean those breakpoints pertaining to a
process that are "automatically cleared on every task switch", which I
presume, happen in cases where TSS registers are used for context
switches (and as I learn is not the case with Linux).

The hw-breakpoint infrastructure in Linux currently implements
per-process breakpoints using 'global' flag but performs the clean-up
after a task-switch using other methods (such as scheduler hooks through
perf-events). All breakpoint requests (kernel or per-process)
is treated as 'global'.

We could change this to become 'local' for every local request (but still
cleanup the breakpoints using scheduler hooks like the way we presently
do), but I think this is an implementation detail and that a ptrace user
need not worry about it. Or do you believe that there's any?

I'm afraid I don't understand your motivation for these read/write tests
on debug control register? Such tests, as in this case, cause unnecessary
panic due to changes in an implementation detail internal to the kernel
without any perceptible difference in functionality.

Thanks,
K.Prasad

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

[Alexandre Julliard]

"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> We could change this to become 'local' for every local request (but still
> cleanup the breakpoints using scheduler hooks like the way we presently
> do), but I think this is an implementation detail and that a ptrace user
> need not worry about it. Or do you believe that there's any?
>
> I'm afraid I don't understand your motivation for these read/write tests
> on debug control register? Such tests, as in this case, cause unnecessary
> panic due to changes in an implementation detail internal to the kernel
> without any perceptible difference in functionality.

Various copy protection schemes such as Safedisc do all sorts of strange
manipulations and checks on the debug registers, in an attempt to make
sure that the app is not being run under a debugger. The Wine exception
tests try to replicate that behavior.

-- 
Alexandre Julliard
julliard@winehq.org

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

[Michael Stefaniuc]

On 02/15/2010 12:57 PM, K.Prasad wrote:
> On Mon, Feb 15, 2010 at 12:05:16AM +0100, Michael Stefaniuc wrote:
>> 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
>>
>
> Okay, so this 0x155 written onto ptrace got converted into 0x2AA -
> basically all requests to 'locally' enable breakpoints in DR0-DR3 (bits
> 0, 2, 4 and 6 of DR7) was converted into a request to 'globally' enable
> (bits 1, 3, 5 and 7) breakpoints.
Ok, so we have two regressions here:
- One fixed by Frederic where breakpoints at address 0x0 weren't allowed 
(Frederic, can you please upstream that fix?).
- The other one with 'locally'/'globally' enabled breakpoints.

> 'Local' breakpoints - here would mean those breakpoints pertaining to a
> process that are "automatically cleared on every task switch", which I
> presume, happen in cases where TSS registers are used for context
> switches (and as I learn is not the case with Linux).
>
> The hw-breakpoint infrastructure in Linux currently implements
> per-process breakpoints using 'global' flag but performs the clean-up
> after a task-switch using other methods (such as scheduler hooks through
> perf-events). All breakpoint requests (kernel or per-process)
> is treated as 'global'.
>
> We could change this to become 'local' for every local request (but still
> cleanup the breakpoints using scheduler hooks like the way we presently
> do), but I think this is an implementation detail and that a ptrace user
> need not worry about it. Or do you believe that there's any?
>
> I'm afraid I don't understand your motivation for these read/write tests
> on debug control register? Such tests, as in this case, cause unnecessary
Like Alexandre said that functionality is used by copy protection 
mechanisms.

> panic due to changes in an implementation detail internal to the kernel
> without any perceptible difference in functionality.
The behavior change is user visible and thus part of the ABI and not 
just an implementation detail.

thanks
bye
	michael

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

[Roland McGrath]

> - The other one with 'locally'/'globally' enabled breakpoints.

There is no "local/global" enablement.  That distinction is meaningless
given the way the kernel uses the hardware.  Which of those bits you set
has no material effect on the watchpoint/trap behavior.  

The only regression is in the observed bit pattern read back from dr7.
To be 100% compatible, the hw_breakpoint ptrace-compatibility front-end
should record the state of the useless bits to report back, so the only
differences from the bit pattern written are whatever ones the real
hardware would have shown from writing dr7 and reading it back.


Thanks,
Roland
 

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

[Frederic Weisbecker]

On Mon, Feb 15, 2010 at 11:47:24AM -0800, Roland McGrath wrote:
> > - The other one with 'locally'/'globally' enabled breakpoints.
> 
> There is no "local/global" enablement.  That distinction is meaningless
> given the way the kernel uses the hardware.  Which of those bits you set
> has no material effect on the watchpoint/trap behavior.  



Yeah.


 
> The only regression is in the observed bit pattern read back from dr7.
> To be 100% compatible, the hw_breakpoint ptrace-compatibility front-end
> should record the state of the useless bits to report back, so the only
> differences from the bit pattern written are whatever ones the real
> hardware would have shown from writing dr7 and reading it back.



Agreed. We have to match the previous ABI, it's a regression.

We have the NULL breakpoint address thing fixed (will push it to Ingo).
We now need to fix the local/global flag storage.

The fastest way to do so is to keep a per thread dr7 variable.
I'm looking at it and will send a fix soon.

We can think about something proper later.

Thanks.

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

[Frederic Weisbecker]

On Mon, Feb 15, 2010 at 12:05:16AM +0100, Michael Stefaniuc wrote:
> 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


Thanks, should work fine for me.



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


OK, will send the second fix soon (the one that fix the dr7 mismatch).

Thanks.

Regression in ptrace (Wine) starting with 2.6.33-rc1, fixes

[Frederic Weisbecker]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

Hi Michael, all,

I still can't build the wine tree (even with the wine faq)
because of the following error:

LD_LIBRARY_PATH="../../libs/wine:$LD_LIBRARY_PATH" ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include  -D__WINESRC__ -D_KERNEL32_  -folocale_rc.res locale_rc.rc
Source: �Q�� a4 51 a4 eb
Unicode:  5341 6708
Back: �̤� a2 cc a4 eb
nls/cht.nls:84:16: Error: String �Q�� does not convert identically to Unicode and back in codepage 950. Try using a Unicode string instead
make[1]: *** [locale_rc.res] Erreur 1

So I can't test these fixes by myself (although I've tested locally
with a home made POKEUSER -> PEEKUSER match test).

Could you please test the following tree? You can pull it on top
of Linus tree, or clone it.

It's here:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	perf/urgent

Thanks,
	Frederic

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

[Michael Stefaniuc]

Thanks Frederic!

On 02/18/2010 06:59 PM, Frederic Weisbecker wrote:
> I still can't build the wine tree (even with the wine faq)
> because of the following error:
>
> LD_LIBRARY_PATH="../../libs/wine:$LD_LIBRARY_PATH" ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include  -D__WINESRC__ -D_KERNEL32_  -folocale_rc.res locale_rc.rc
> Source: �Q�� a4 51 a4 eb
> Unicode:  5341 6708
> Back: �̤� a2 cc a4 eb
> nls/cht.nls:84:16: Error: String �Q�� does not convert identically to Unicode and back in codepage 950. Try using a Unicode string instead
> make[1]: *** [locale_rc.res] Erreur 1
Wow, what Wine version are you trying to build? That was a known
regression but it was fixed 1-2 days later.

> So I can't test these fixes by myself (although I've tested locally
> with a home made POKEUSER ->  PEEKUSER match test).
>
> Could you please test the following tree? You can pull it on top
> of Linus tree, or clone it.
>
> It's here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	perf/urgent
I have just applied your two patches on top of current git. I have run 
the specific test (make exception.ok) as well as full winetest.exe run:
The ptrace regressions impacting Wine are all fixed.

Thanks again for the quick resolution.
bye
         michael

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

[Alexandre Julliard]

Michael Stefaniuc <mstefani@redhat.com> writes:

> On 02/18/2010 06:59 PM, Frederic Weisbecker wrote:
>> I still can't build the wine tree (even with the wine faq)
>> because of the following error:
>>
>> LD_LIBRARY_PATH="../../libs/wine:$LD_LIBRARY_PATH" ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include  -D__WINESRC__ -D_KERNEL32_  -folocale_rc.res locale_rc.rc
>> Source: �Q�� a4 51 a4 eb
>> Unicode:  5341 6708
>> Back: �̤� a2 cc a4 eb
>> nls/cht.nls:84:16: Error: String �Q�� does not convert identically to Unicode and back in codepage 950. Try using a Unicode string instead
>> make[1]: *** [locale_rc.res] Erreur 1
> Wow, what Wine version are you trying to build? That was a known
> regression but it was fixed 1-2 days later.

This is usually because that you have an old libwine.so lying around
somewhere.

-- 
Alexandre Julliard
julliard@winehq.org

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

[Frederic Weisbecker]

On Thu, Feb 18, 2010 at 08:41:04PM +0100, Alexandre Julliard wrote:
> Michael Stefaniuc <mstefani@redhat.com> writes:
> 
> > On 02/18/2010 06:59 PM, Frederic Weisbecker wrote:
> >> I still can't build the wine tree (even with the wine faq)
> >> because of the following error:
> >>
> >> LD_LIBRARY_PATH="../../libs/wine:$LD_LIBRARY_PATH" ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include  -D__WINESRC__ -D_KERNEL32_  -folocale_rc.res locale_rc.rc
> >> Source: �Q�� a4 51 a4 eb
> >> Unicode:  5341 6708
> >> Back: �̤� a2 cc a4 eb
> >> nls/cht.nls:84:16: Error: String �Q�� does not convert identically to Unicode and back in codepage 950. Try using a Unicode string instead
> >> make[1]: *** [locale_rc.res] Erreur 1
> > Wow, what Wine version are you trying to build? That was a known
> > regression but it was fixed 1-2 days later.
> 
> This is usually because that you have an old libwine.so lying around
> somewhere.


Ah probably. I should have removed my previous wine setup before doing
this.

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

[Frederic Weisbecker]

On Thu, Feb 18, 2010 at 08:27:41PM +0100, Michael Stefaniuc wrote:
> Thanks Frederic!
>
> On 02/18/2010 06:59 PM, Frederic Weisbecker wrote:
>> I still can't build the wine tree (even with the wine faq)
>> because of the following error:
>>
>> LD_LIBRARY_PATH="../../libs/wine:$LD_LIBRARY_PATH" ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include  -D__WINESRC__ -D_KERNEL32_  -folocale_rc.res locale_rc.rc
>> Source: �Q�� a4 51 a4 eb
>> Unicode:  5341 6708
>> Back: �̤� a2 cc a4 eb
>> nls/cht.nls:84:16: Error: String �Q�� does not convert identically to Unicode and back in codepage 950. Try using a Unicode string instead
>> make[1]: *** [locale_rc.res] Erreur 1
> Wow, what Wine version are you trying to build? That was a known
> regression but it was fixed 1-2 days later.
>


Hmm, it's a git clone from the wine repo, probably 5 days old.



>> So I can't test these fixes by myself (although I've tested locally
>> with a home made POKEUSER ->  PEEKUSER match test).
>>
>> Could you please test the following tree? You can pull it on top
>> of Linus tree, or clone it.
>>
>> It's here:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
>> 	perf/urgent
> I have just applied your two patches on top of current git. I have run  
> the specific test (make exception.ok) as well as full winetest.exe run:
> The ptrace regressions impacting Wine are all fixed.



Great!



> Thanks again for the quick resolution.
> bye


And thanks a lot for all your testing.
I'm adding your tested-by and will push the patches.

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

[PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

When the user enables breakpoints through dr7, he can choose
between "local" or "global" enable bits but given how linux is
implemented, both have the same effect.

That said we don't keep track how the user enabled the breakpoints
so when the user requests the dr7 value, we only translate the
"enabled" status using the global enabled bits. It means that if
the user enabled a breakpoint using the local enabled bit, reading
back dr7 will set the global bit and clear the local one.

Apps like Wine expect a full dr7 POKEUSER/PEEKUSER match for emulated
softwares that implement old reverse engineering protection schemes.

We fix that by keeping track of the whole dr7 value given by the user
in the thread structure to drop this bug. We'll think about
something more proper later.

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/include/asm/processor.h |    2 ++
 arch/x86/kernel/ptrace.c         |    7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index fc801ba..b753ea5 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -450,6 +450,8 @@ struct thread_struct {
 	struct perf_event	*ptrace_bps[HBP_NUM];
 	/* Debug status used for traps, single steps, etc... */
 	unsigned long           debugreg6;
+	/* Keep track of the exact dr7 value set by the user */
+	unsigned long           ptrace_dr7;
 	/* Fault info: */
 	unsigned long		cr2;
 	unsigned long		trap_no;
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;
 	}
 	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);
+		if (!rc)
+			thread->ptrace_dr7 = val;
+	}
 
 ret_path:
 	return rc;
-- 
1.6.2.3

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[K.Prasad]

On Thu, Feb 18, 2010 at 07:00:01PM +0100, Frederic Weisbecker wrote:
> When the user enables breakpoints through dr7, he can choose
> between "local" or "global" enable bits but given how linux is
> implemented, both have the same effect.
> 
> That said we don't keep track how the user enabled the breakpoints
> so when the user requests the dr7 value, we only translate the
> "enabled" status using the global enabled bits. It means that if
> the user enabled a breakpoint using the local enabled bit, reading
> back dr7 will set the global bit and clear the local one.
> 
> Apps like Wine expect a full dr7 POKEUSER/PEEKUSER match for emulated
> softwares that implement old reverse engineering protection schemes.
> 
> We fix that by keeping track of the whole dr7 value given by the user
> in the thread structure to drop this bug. We'll think about
> something more proper later.
> 
> 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/include/asm/processor.h |    2 ++
>  arch/x86/kernel/ptrace.c         |    7 +++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index fc801ba..b753ea5 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -450,6 +450,8 @@ struct thread_struct {
>  	struct perf_event	*ptrace_bps[HBP_NUM];
>  	/* Debug status used for traps, single steps, etc... */
>  	unsigned long           debugreg6;
> +	/* Keep track of the exact dr7 value set by the user */
> +	unsigned long           ptrace_dr7;
>  	/* Fault info: */
>  	unsigned long		cr2;
>  	unsigned long		trap_no;
> 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;
>  	}
>  	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);
> +		if (!rc)
> +			thread->ptrace_dr7 = val;
> +	}
> 
>  ret_path:
>  	return rc;


So, the thread's copy of DR7 (in thread->ptrace_dr7) stores the
requested data even if the 'write' onto DR7 i.e. ptrace_write_dr7()
failed. This can be the other way round i.e. populate the thread's copy
of DR7 only if the write was successful.

I think it will be in consonance with the v2.6.32 behaviour as well. For
instance, in the code snippet from ptrace_set_debugreg() in v2.6.32
below:
                for (i = 0; i < 4; i++)
                        if ((DR7_MASK >> ((data >> (16 + 4*i)) & 0xf)) & 1)
                                return -EIO;
                child->thread.debugreg7 = data;

The thread's copy of DR7 is populated only if the incoming data is
found to be valid.

Thanks,
K.Prasad

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

2010/2/19 K.Prasad <prasad@linux.vnet.ibm.com>:
> So, the thread's copy of DR7 (in thread->ptrace_dr7) stores the
> requested data even if the 'write' onto DR7 i.e. ptrace_write_dr7()
> failed. This can be the other way round i.e. populate the thread's copy
> of DR7 only if the write was successful.



No. We store the new dr7 value only if ptrace_set_dr7() didn't fail.



> I think it will be in consonance with the v2.6.32 behaviour as well. For
> instance, in the code snippet from ptrace_set_debugreg() in v2.6.32
> below:
>                for (i = 0; i < 4; i++)
>                        if ((DR7_MASK >> ((data >> (16 + 4*i)) & 0xf)) & 1)
>                                return -EIO;
>                child->thread.debugreg7 = data;
>
> The thread's copy of DR7 is populated only if the incoming data is
> found to be valid.


This is also what does this patch. thread->ptrace_dr7 is only
changed if ptrace_set_dr7() succeeded.

Thanks.

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[K.Prasad]

On Fri, Feb 19, 2010 at 04:34:03PM +0100, Frederic Weisbecker wrote:
> 2010/2/19 K.Prasad <prasad@linux.vnet.ibm.com>:
> > So, the thread's copy of DR7 (in thread->ptrace_dr7) stores the
> > requested data even if the 'write' onto DR7 i.e. ptrace_write_dr7()
> > failed. This can be the other way round i.e. populate the thread's copy
> > of DR7 only if the write was successful.
> 
> 
> 
> No. We store the new dr7 value only if ptrace_set_dr7() didn't fail.
> 
> 
> 
> > I think it will be in consonance with the v2.6.32 behaviour as well. For
> > instance, in the code snippet from ptrace_set_debugreg() in v2.6.32
> > below:
> >                for (i = 0; i < 4; i++)
> >                        if ((DR7_MASK >> ((data >> (16 + 4*i)) & 0xf)) & 1)
> >                                return -EIO;
> >                child->thread.debugreg7 = data;
> >
> > The thread's copy of DR7 is populated only if the incoming data is
> > found to be valid.
> 
> 
> This is also what does this patch. thread->ptrace_dr7 is only
> changed if ptrace_set_dr7() succeeded.
> 
> Thanks.

hmmh...I see...looks like I experienced single-bit ECC as I read
the patch :-) Yes, let debugreg7 store values only when a valid bkpt
request comes in.

Thanks,
K.Prasad

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

On Fri, Feb 19, 2010 at 11:28:59PM +0530, K.Prasad wrote:
> On Fri, Feb 19, 2010 at 04:34:03PM +0100, Frederic Weisbecker wrote:
> > 2010/2/19 K.Prasad <prasad@linux.vnet.ibm.com>:
> > > So, the thread's copy of DR7 (in thread->ptrace_dr7) stores the
> > > requested data even if the 'write' onto DR7 i.e. ptrace_write_dr7()
> > > failed. This can be the other way round i.e. populate the thread's copy
> > > of DR7 only if the write was successful.
> > 
> > 
> > 
> > No. We store the new dr7 value only if ptrace_set_dr7() didn't fail.
> > 
> > 
> > 
> > > I think it will be in consonance with the v2.6.32 behaviour as well. For
> > > instance, in the code snippet from ptrace_set_debugreg() in v2.6.32
> > > below:
> > >                for (i = 0; i < 4; i++)
> > >                        if ((DR7_MASK >> ((data >> (16 + 4*i)) & 0xf)) & 1)
> > >                                return -EIO;
> > >                child->thread.debugreg7 = data;
> > >
> > > The thread's copy of DR7 is populated only if the incoming data is
> > > found to be valid.
> > 
> > 
> > This is also what does this patch. thread->ptrace_dr7 is only
> > changed if ptrace_set_dr7() succeeded.
> > 
> > Thanks.
> 
> hmmh...I see...looks like I experienced single-bit ECC as I read
> the patch :-) Yes, let debugreg7 store values only when a valid bkpt
> request comes in.



Great :) I was feeling uncomfortable to push that out without
your ack. May I add it?

Thanks.

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[K.Prasad]

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

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

2010/2/19 K.Prasad <prasad@linux.vnet.ibm.com>:
> 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?


Not in this set. This is an urgent fix, we shouldn't take corner risks
so late in the process.

Also, this thread->ptrace_dr7 is a temporary thing, a hack to fix a
regression without
taking too much risk. But we need something more proper in the longer term, like
storing this information in the arch_hw_breakpoint (we shouldn't bloat
the thread
structure with such mostly unused field).

I have some ideas to get the arch_hw_breakpoint more "arch", to
register breakpoints
using arch informations only, so that we don't depend on a generic
breakpoint attribute
conversion for ptrace, which may be too limited for tricky arch
breakpoint implementations,
and an unecessary midlayer there.
I'll tell you more while answering to your ppc breakpoint implementation.



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


Yeah we can do that from ptrace_write_dr7(). Either way.
It's just more quick to do that here. I did not think much
about it as I don't think about ptrace_dr7 as a long term field.

Thanks.


>> +             if (!rc)
>> +                     thread->ptrace_dr7 = val;
>> +     }
>>
>>  ret_path:
>>       return rc;

Re: [PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

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.

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
 

[GIT PULL] hw-breakpoint regression fixes

[Frederic Weisbecker]

Ingo,

Please pull the perf/urgent branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	perf/urgent

Thanks,
	Frederic
---

Frederic Weisbecker (2):
      hw-breakpoints: Accept breakpoints on NULL address
      hw-breakpoint: Keep track of dr7 local enable bits


 arch/x86/include/asm/processor.h |    2 ++
 arch/x86/kernel/hw_breakpoint.c  |   30 +++++++-----------------------
 arch/x86/kernel/ptrace.c         |    7 +++++--
 3 files changed, 14 insertions(+), 25 deletions(-)

Re: [GIT PULL] hw-breakpoint regression fixes

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Ingo,
> 
> Please pull the perf/urgent branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	perf/urgent
> 
> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (2):
>       hw-breakpoints: Accept breakpoints on NULL address
>       hw-breakpoint: Keep track of dr7 local enable bits
> 
> 
>  arch/x86/include/asm/processor.h |    2 ++
>  arch/x86/kernel/hw_breakpoint.c  |   30 +++++++-----------------------
>  arch/x86/kernel/ptrace.c         |    7 +++++--
>  3 files changed, 14 insertions(+), 25 deletions(-)

Pulled, thanks a lot!

	Ingo

[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

[PATCH 2/2] hw-breakpoint: Keep track of dr7 local enable bits

[Frederic Weisbecker]

When the user enables breakpoints through dr7, he can choose
between "local" or "global" enable bits but given how linux is
implemented, both have the same effect.

That said we don't keep track how the user enabled the breakpoints
so when the user requests the dr7 value, we only translate the
"enabled" status using the global enabled bits. It means that if
the user enabled a breakpoint using the local enabled bit, reading
back dr7 will set the global bit and clear the local one.

Apps like Wine expect a full dr7 POKEUSER/PEEKUSER match for emulated
softwares that implement old reverse engineering protection schemes.

We fix that by keeping track of the whole dr7 value given by the user
in the thread structure to drop this bug. We'll think about
something more proper later.

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>
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/include/asm/processor.h |    2 ++
 arch/x86/kernel/ptrace.c         |    7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index fc801ba..b753ea5 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -450,6 +450,8 @@ struct thread_struct {
 	struct perf_event	*ptrace_bps[HBP_NUM];
 	/* Debug status used for traps, single steps, etc... */
 	unsigned long           debugreg6;
+	/* Keep track of the exact dr7 value set by the user */
+	unsigned long           ptrace_dr7;
 	/* Fault info: */
 	unsigned long		cr2;
 	unsigned long		trap_no;
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;
 	}
 	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);
+		if (!rc)
+			thread->ptrace_dr7 = val;
+	}
 
 ret_path:
 	return rc;
-- 
1.6.2.3

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