{make,set,swap}context broken on powerpc32

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

{make,set,swap}context broken on powerpc32

Jakub Jelinek
Hi!

The following testcase crashes on powerpc32 with CVS glibc (but glibc 2.5 as
well) and 2.6.18 kernel, when glibc is configured for 2.6.0+ kernel
(i.e. __ASSUME_SWAPCONTEXT_SYSCALL).  The problem is in clobberring r2 (== TLS)
register.
One thing is that swapcontext syscall restores r2 (for 32-bit processes)
from the saved registers.  As it is used by swapcontext and setcontext
syscalls, that in itself is a problem - can't you getcontext in one
thread and setcontext in another one?
Additionally, it seems makecontext call doesn't preserve any registers
that getcontext saved, as it overwrites uc_mcontext.uc_regs pointer
(on this testcase getcontext created context has
uc_mcontext.uc_regs pointing to uc_reg_space + 12 while makecontext
resets that to uc_mcontext.uc_regs).
The combination of those two problems causes the failure of this testcase,
but I think for both the problems we can construct a testcase separately.

For makecontext, I wonder if we just shouldn't memmove the register values
around if we change the pointer.  For the swapcontext/setcontext, there
is a bigger problem, either we change the kernel not to restore r2,
or we could e.g. compare the saved r2 with the current r2, if equal to what
we do ATM, otherwise copy the whole ucontext_t into a temporary, change
r2 in it and only then do the swapcontext syscall.

#define _XOPEN_SOURCE 600
#include <stdlib.h>
#include <stdio.h>
#include <ucontext.h>

ucontext_t oucp, ucp;
char st1[8192];
__thread int thr;

void
cf (int i)
{
  if (i != 78 || thr != 94)
    {
      printf ("i %d thr %d\n", i, thr);
      exit (1);
    }
  exit (0);
}

int
main (void)
{
  if (getcontext (&oucp) != 0 || getcontext (&ucp) != 0)
    {
      puts ("getcontext failed");
      return 1;
    }
  thr = 94;
  ucp.uc_link = &oucp;
  ucp.uc_stack.ss_sp = st1;
  ucp.uc_stack.ss_size = sizeof st1;
  makecontext (&ucp, (void (*) ()) cf, 1, 78);
  if (setcontext (&ucp) != 0)
    {
      puts ("setcontext failed");
      return 1;
    }
  return 0;
}

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Steven Munroe
Jakub Jelinek wrote:
>Hi!
>
>The following testcase crashes on powerpc32 with CVS glibc (but glibc 2.5 as
>well) and 2.6.18 kernel, when glibc is configured for 2.6.0+ kernel
>(i.e. __ASSUME_SWAPCONTEXT_SYSCALL).  The problem is in clobberring r2 (== TLS)
>register.
>  
The kernel should not restore r2 from the ucontext and should keep the
r2 for the thread. The problem state implementation (#undef
__ASSUME_SWAPCONTEXT_SYSCAL) leaves r2 untouched.

Seems that __ASSUME_SWAPCONTEXT_SYSCALL does not get set unless
--enable-kernel=2.6.0+ is configured so my default testing  does not
hit. Also powerpc64 is using the older __ASSUME_NEW_RT_SIGRETURN_SYSCALL
and does not hit this.
>One thing is that swapcontext syscall restores r2 (for 32-bit processes)
>from the saved registers.  As it is used by swapcontext and setcontext
>syscalls, that in itself is a problem - can't you getcontext in one
>thread and setcontext in another one?
>  

The getcontext/setcontext context is more like a coroutine call and can
not (should not) change the thread pointer (or the thread struct or the
kernels internal thread). So it is more like a N to M thread model.
>Additionally, it seems makecontext call doesn't preserve any registers
>that getcontext saved, as it overwrites uc_mcontext.uc_regs pointer
>(on this testcase getcontext created context has
>uc_mcontext.uc_regs pointing to uc_reg_space + 12 while makecontext
>resets that to uc_mcontext.uc_regs).
>The combination of those two problems causes the failure of this testcase,
>but I think for both the problems we can construct a testcase separately.
>  
Its more complicated then that. When we added Altivec we had to force
quadword aligment of the uc_reg_space even when the ucontext struct
itself was not and some kernels did not get the set uc_regs_ptr at all.
So the  sequence:
    addi    r11,r3,_UC_REG_SPACE+12 // round up if not already quadword
    clrrwi  r11,r11,4  // round down to quadword
    stw    r11,_UC_REGS_PTR(r3)
Forces a round up to quadword and makes sure the uc_regs_ptr is set. If
you look at a getcontext you will see the same sequence. So for a
properly created ucontext makecontext is not changing the uc_regs_ptr at
all.

So this can only be a problem if the kernel is following different
rules. For example if the kernel was not forcing quadword alignment for
uc_reg_space or was only force quadword for PPC_FEATURE_HAS_ALTIVEC.

makecontext does overwrite the parameter registers the PC and LR to set
the call to the target function. Otherwise non-volitile registes are not
changed.
>For makecontext, I wonder if we just shouldn't memmove the register values
>around if we change the pointer.  For the swapcontext/setcontext, there
>  
>is a bigger problem, either we change the kernel not to restore r2,
>or we could e.g. compare the saved r2 with the current r2, if equal to what
>we do ATM, otherwise copy the whole ucontext_t into a temporary, change
>r2 in it and only then do the swapcontext syscall.
>
>  
We should not have to memmove the registers unless they are unaligned,
which should never happen.

Perhaps we should use __ASSUME_SWAPCONTEXT_SYSCALL to tell makecontext
to assume us_regs is valid (set and aligned) and use it as is to set up
the parameter for the target function?

But it is a problem that the kernel is modifying r2. The should be fixed
in the kernel.

>#define _XOPEN_SOURCE 600
>#include <stdlib.h>
>#include <stdio.h>
>#include <ucontext.h>
>
>ucontext_t oucp, ucp;
>char st1[8192];
>__thread int thr;
>
>void
>cf (int i)
>{
>  if (i != 78 || thr != 94)
>    {
>      printf ("i %d thr %d\n", i, thr);
>      exit (1);
>    }
>  exit (0);
>}
>
>int
>main (void)
>{
>  if (getcontext (&oucp) != 0 || getcontext (&ucp) != 0)
>    {
>      puts ("getcontext failed");
>      return 1;
>    }
>  thr = 94;
>  ucp.uc_link = &oucp;
>  ucp.uc_stack.ss_sp = st1;
>  ucp.uc_stack.ss_size = sizeof st1;
>  makecontext (&ucp, (void (*) ()) cf, 1, 78);
>  if (setcontext (&ucp) != 0)
>    {
>      puts ("setcontext failed");
>      return 1;
>    }
>  return 0;
>}
>
> Jakub
>  

Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Daniel Jacobowitz-2
On Tue, Dec 12, 2006 at 04:16:22PM -0600, Steven Munroe wrote:
> >One thing is that swapcontext syscall restores r2 (for 32-bit processes)
> >from the saved registers.  As it is used by swapcontext and setcontext
> >syscalls, that in itself is a problem - can't you getcontext in one
> >thread and setcontext in another one?
> >  
>
> The getcontext/setcontext context is more like a coroutine call and can
> not (should not) change the thread pointer (or the thread struct or the
> kernels internal thread). So it is more like a N to M thread model.

If you getcontext in one thread and setcontext in another, even if you
leave r2 alone, aren't you going to have other subtler problems?  I
doubt GCC expects function calls to clobber the address of __thread
variables.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Ulrich Drepper
Daniel Jacobowitz wrote:
> If you getcontext in one thread and setcontext in another, even if you
> leave r2 alone, aren't you going to have other subtler problems?  I
> doubt GCC expects function calls to clobber the address of __thread
> variables.

Then your code is broken.  It must be possible to use contexts in
different threads.  Since this is all explicit scheduling it's easy
enough to avoid constructs where the generated code depends on __thread
not changing.  For explicit __thread variables it is the programmer's
responsibility to avoid them.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
Ulrich Drepper writes:

> Then your code is broken.  It must be possible to use contexts in
> different threads.  Since this is all explicit scheduling it's easy
> enough to avoid constructs where the generated code depends on __thread
> not changing.  For explicit __thread variables it is the programmer's
> responsibility to avoid them.

Does that last sentence mean that it is the programmer's
responsibility to avoid using __thread variables entirely if he/she is
using setcontext or swapcontext anywhere in his/her program?  Or did
you mean something different?

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Ulrich Drepper
Paul Mackerras wrote:
> Does that last sentence mean that it is the programmer's
> responsibility to avoid using __thread variables entirely if he/she is
> using setcontext or swapcontext anywhere in his/her program?  Or did
> you mean something different?

It means they must be avoided if they are needed across scheduling
points (i.e., makecontext calls).  Using __thread (e.g., via OpenMP) is
perfectly fine as long as the entire block of code cannot be interrupted
by a makecontext call.  And these calls don't sneak up on you, it's
cooperative scheduling.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
In reply to this post by Jakub Jelinek
Jakub Jelinek writes:

> The following testcase crashes on powerpc32 with CVS glibc (but glibc 2.5 as
> well) and 2.6.18 kernel, when glibc is configured for 2.6.0+ kernel
> (i.e. __ASSUME_SWAPCONTEXT_SYSCALL).  The problem is in clobberring r2 (== TLS)
> register.
> One thing is that swapcontext syscall restores r2 (for 32-bit processes)
> from the saved registers.

I find it hard to see how that could be happening, because the kernel
code explicitly preserves r2.  In arch/powerpc/kernel/signal_32.c,
sys_swapcontext calls do_setcontext with `sig' == 0, which calls
restore_user_regs with the same `sig' argument, which then does this:

        /*
         * restore general registers but not including MSR or SOFTE. Also
         * take care of keeping r2 (TLS) intact if not a signal
         */
        if (!sig)
                save_r2 = (unsigned int)regs->gpr[2];
        err = restore_general_regs(regs, sr);
        err |= __get_user(msr, &sr->mc_gregs[PT_MSR]);
        if (!sig)
                regs->gpr[2] = (unsigned long) save_r2;

The 64-bit sys_swapcontext (in signal_64.c) similarly avoids modifying
r13, using a slightly different mechanism.

That's assuming that glibc is in fact using the sys_swapcontext system
call.  If it's using sys_rt_sigreturn instead, then yes that will
restore r2 from the saved registers.

Could you strace your test program and see whether it is using
sys_swapcontext or sys_rt_sigreturn?

I just wrote a little test program to call sys_swapcontext directly
and check what happens to r2, and it appears to be doing the right
thing (i.e. sys_swapcontext with non-NULL second argument is not
modifying r2).  This is with a 2.6.18-rc7 ppc64 kernel, and a 32-bit
user process.  I'll try a ppc32 kernel shortly.

Regards,
Paul.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
I wrote:

> I just wrote a little test program to call sys_swapcontext directly
> and check what happens to r2, and it appears to be doing the right
> thing (i.e. sys_swapcontext with non-NULL second argument is not
> modifying r2).  This is with a 2.6.18-rc7 ppc64 kernel, and a 32-bit
> user process.  I'll try a ppc32 kernel shortly.

And indeed with a 2.6.18 ppc32 kernel the system call is doing the
right thing also.  Also, a similar test with a 64-bit process on a
64-bit kernel confirmed that the 64-bit sys_swapcontext is not
altering r13.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Jakub Jelinek
In reply to this post by Paul Mackerras
On Wed, Dec 13, 2006 at 01:43:31PM +1100, Paul Mackerras wrote:

> I find it hard to see how that could be happening, because the kernel
> code explicitly preserves r2.  In arch/powerpc/kernel/signal_32.c,
> sys_swapcontext calls do_setcontext with `sig' == 0, which calls
> restore_user_regs with the same `sig' argument, which then does this:
>
> /*
> * restore general registers but not including MSR or SOFTE. Also
> * take care of keeping r2 (TLS) intact if not a signal
> */
> if (!sig)
> save_r2 = (unsigned int)regs->gpr[2];
> err = restore_general_regs(regs, sr);
> err |= __get_user(msr, &sr->mc_gregs[PT_MSR]);
> if (!sig)
> regs->gpr[2] = (unsigned long) save_r2;
>
> The 64-bit sys_swapcontext (in signal_64.c) similarly avoids modifying
> r13, using a slightly different mechanism.
>
> That's assuming that glibc is in fact using the sys_swapcontext system
> call.  If it's using sys_rt_sigreturn instead, then yes that will
> restore r2 from the saved registers.
>
> Could you strace your test program and see whether it is using
> sys_swapcontext or sys_rt_sigreturn?

It uses sys_swapcontext to fill it up (i.e. in getcontext calls)
and apparently sys_rt_sigreturn in setcontext (sorry I haven't noticed that),
will look why:

strace /tmp/e2
execve("/tmp/e2", ["/tmp/e2"], [/* 26 vars */]) = 0
brk(0)                                  = 0x10020000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY)      = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=135453, ...}) = 0
mmap(NULL, 135453, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf7fc0000
close(3)                                = 0
open("/lib/libc.so.6", O_RDONLY)        = 3
read(3, "\177ELF\1\2\1\0\0\0\0\0\0\0\0\0\0\3\0\24\0\0\0\1\17H\340"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=1793784, ...}) = 0
mmap(0xf470000, 1520028, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xf470000
mmap(0xf5d0000, 131072, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x160000) = 0xf5d0000
close(3)                                = 0
mprotect(0xf5d0000, 65536, PROT_READ)   = 0
mprotect(0xffe0000, 65536, PROT_READ)   = 0
munmap(0xf7fc0000, 135453)              = 0
swapcontext(0x1001319c, 0)              = 0
swapcontext(0x10010cfc, 0)              = 0
rt_sigreturn(0x10010cfc)                = 78
--- SIGSEGV (Segmentation fault) @ 0 (0) ---
+++ killed by SIGSEGV +++

uc_mcontext.uc_regs pointer the swapcontext call creates is not
aligned though:
p ucp
$1 = {uc_flags = 0, uc_link = 0x0, uc_stack = {ss_sp = 0x0, ss_flags = 0, ss_size = 0}, uc_pad = {0, 0, 0, 0, 0, 0, 0},
  uc_mcontext = {regs = 0x10010dbc, uc_regs = 0x10010dbc}, uc_sigmask = {__val = {0 <repeats 32 times>}},
  uc_reg_space = ... }

and makecontext realigns it:

$2 = {uc_flags = 0, uc_link = 0x1001319c, uc_stack = {ss_sp = 0x1001119c, ss_flags = 0, ss_size = 8192}, uc_pad = {0, 0, 0, 0, 0, 0, 0},
  uc_mcontext = {regs = 0x10010db0, uc_regs = 0x10010db0}, uc_sigmask = {__val = {0 <repeats 32 times>}},
  uc_reg_space = ... }

thus loosing content of all registers from getcontext except those which it
explicitly overrode.

        Jakub
Reply | Threaded
Open this post in threaded view
|

[PATCH] Re: {make,set,swap}context broken on powerpc32

Jakub Jelinek
On Wed, Dec 13, 2006 at 09:47:57AM +0100, Jakub Jelinek wrote:
> > Could you strace your test program and see whether it is using
> > sys_swapcontext or sys_rt_sigreturn?
>
> It uses sys_swapcontext to fill it up (i.e. in getcontext calls)
> and apparently sys_rt_sigreturn in setcontext (sorry I haven't noticed that),
> will look why:

The reason for this is a bug in glibc, while getcontext/swapcontext both
include kernel-features.h and thus __ASSUME_SWAPCONTEXT_SYSCALL is defined
in --enable-kernel=2.6.9 builds, setcontext does not and therefore it
always uses other methods.  Sorry for thinking it was making a swapcontext
syscall even the third time, I saw it is making some syscall under gdb
and looking at the source I it appeared it would use swapcontext.
While this cures the testcase I posted (r2 is no longer restored by
swapcontext), I still don't like the realignment.  Either the kernel
has a bug and should always align, or makecontext needs to memmove when it
realigns.

2006-12-13  Jakub Jelinek  <[hidden email]>

        * sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S: Include
        kernel-features.h.

--- libc/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S.jj 2005-12-30 09:04:17.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S 2006-12-13 09:49:29.000000000 +0100
@@ -1,5 +1,5 @@
 /* Jump to a new context.
-   Copyright (C) 2002, 2004, 2005 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -20,6 +20,7 @@
 #include <sysdep.h>
 #include <rtld-global-offsets.h>
 #include <shlib-compat.h>
+#include <kernel-features.h>
 
 #define __ASSEMBLY__
 #include <asm/ptrace.h>


        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
In reply to this post by Jakub Jelinek
Jakub Jelinek writes:

> uc_mcontext.uc_regs pointer the swapcontext call creates is not
> aligned though:

Now that I can believe, since the register saving code is derived from
the code used to construct signal frames, and therefore assumes that
the ucontext is 16-byte aligned.  So yes, that's a kernel bug.

Note that sys_swapcontext can happily use, as its second argument, a
ucontext where the uc_mcontext.uc_regs field is not 16-byte aligned,
though, so glibc could work around the kernel bug by doing the memmove
in makecontext as you suggest.

When I fix the kernel bug, which of these two options would you
prefer?

(a) the kernel sets ucp->uc_mcontext.uc_regs to the value it uses now,
rounded up to a 16-byte boundary, and stores the registers there; or

(b) the kernel relies on glibc to have set ucp->uc_mcontext.uc_regs
appropriately and stores the registers where it points to (i.e. the
kernel does not alter ucp->uc_mcontext.uc_regs).

Regards,
Paul.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Steven Munroe
Paul Mackerras wrote:

>Jakub Jelinek writes:
>
>  
>>uc_mcontext.uc_regs pointer the swapcontext call creates is not
>>aligned though:
>>    
>
>Now that I can believe, since the register saving code is derived from
>the code used to construct signal frames, and therefore assumes that
>the ucontext is 16-byte aligned.  So yes, that's a kernel bug.
>
>Note that sys_swapcontext can happily use, as its second argument, a
>ucontext where the uc_mcontext.uc_regs field is not 16-byte aligned,
>though, so glibc could work around the kernel bug by doing the memmove
>in makecontext as you suggest.
>
>  
I would like to avoid any memmove by aligning the regs buffer properly
in the first place.

>When I fix the kernel bug, which of these two options would you
>prefer?
>
>(a) the kernel sets ucp->uc_mcontext.uc_regs to the value it uses now,
>rounded up to a 16-byte boundary, and stores the registers there; or
>
>  
When the kernel allocates the ucontext (signals) , the kernel should
align the regs buffer and set uc_mcontext.uc_regs appropriately.
>(b) the kernel relies on glibc to have set ucp->uc_mcontext.uc_regs
>appropriately and stores the registers where it points to (i.e. the
>kernel does not alter ucp->uc_mcontext.uc_regs).
>
>  
When the user or glibc allocate the ucontext (get/swapcontext), glibc
should align the regs buffer and set uc_mcontext.uc_regs. Then
set/makecontext can safely use uc_mcontext.uc_regs (#ifdef
__ASSUME_SWAPCONTEXT_SYSCALL) without forcing alignment.

Reply | Threaded
Open this post in threaded view
|

[PATCH] Re: {make,set,swap}context broken on powerpc32

Steven Munroe
In reply to this post by Jakub Jelinek
Jakub Jelinek wrote:

>On Wed, Dec 13, 2006 at 09:47:57AM +0100, Jakub Jelinek wrote:
>  
>>>Could you strace your test program and see whether it is using
>>>sys_swapcontext or sys_rt_sigreturn?
>>>      
>>It uses sys_swapcontext to fill it up (i.e. in getcontext calls)
>>and apparently sys_rt_sigreturn in setcontext (sorry I haven't noticed that),
>>will look why:
>>    
>
>The reason for this is a bug in glibc, while getcontext/swapcontext both
>include kernel-features.h and thus __ASSUME_SWAPCONTEXT_SYSCALL is defined
>in --enable-kernel=2.6.9 builds, setcontext does not and therefore it
>always uses other methods.  Sorry for thinking it was making a swapcontext
>syscall even the third time, I saw it is making some syscall under gdb
>and looking at the source I it appeared it would use swapcontext.
>While this cures the testcase I posted (r2 is no longer restored by
>swapcontext), I still don't like the realignment.  Either the kernel
>has a bug and should always align, or makecontext needs to memmove when it
>realigns.
>
>  
This patch is more complete where it forces alignment for of uc_regs for
__ASSUME_SWAPCONTEXT_SYSCALL version of  __getcontext and swapcontext
and allows makecontext to assume the uc_regs is set and aligned. This
version passes Jakub's test case on on the 2.6 kernels I have tried so far.

We still need to verify that on ppc970 the kernel is aligning the
uc_regs pointer when VMX regs are present.

2006-12-13  Steven Munroe  <[hidden email]>

        * sysdeps/unix/sysv/linux/powerpc/powerpc32/getcontext.S
        [__ASSUME_SWAPCONTEXT_SYSCALL] (__getcontext): Align reg_space
        and set uc_regs.
        * sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S
        [__ASSUME_SWAPCONTEXT_SYSCALL] (__setcontext): Align reg_space
        and set uc_regs.
        * sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext.S
        [__ASSUME_SWAPCONTEXT_SYSCALL] (__swapcontext): Align reg_space
        and set uc_regs.
        * sysdeps/unix/sysv/linux/powerpc/powerpc32/makecontext.S
        (__makecontext): Assume uc_regs set and aligned.

diff -urN libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/getcontext.S libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/getcontext.S
--- libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/getcontext.S 2005-12-29 15:02:31.000000000 -0600
+++ libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/getcontext.S 2006-12-13 17:11:08.763208616 -0600
@@ -1,5 +1,5 @@
 /* Save current context.
-   Copyright (C) 2002, 2004, 2005 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -36,6 +36,10 @@
 #ifdef __ASSUME_SWAPCONTEXT_SYSCALL
  .section ".text";
 ENTRY (__getcontext)
+/* Insure that the _UC_REGS starts on a quadword boundary.  */
+ addi r0,r3,_UC_REG_SPACE+12
+ clrrwi  r0,r0,4
+ stw r0,_UC_REGS_PTR(r3)
  li r4,0
  li r5,_UC_SIZE_2_3_4;
  DO_CALL (SYS_ify (swapcontext));
diff -urN libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/makecontext.S libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/makecontext.S
--- libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/makecontext.S 2006-01-06 21:51:11.000000000 -0600
+++ libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/makecontext.S 2006-12-13 10:27:02.000000000 -0600
@@ -26,9 +26,7 @@
 
 ENTRY(__makecontext)
  /* Set up the first 7 args to the function in its registers */
- addi r11,r3,_UC_REG_SPACE+12
- clrrwi  r11,r11,4
- stw r11,_UC_REGS_PTR(r3)
+ lwz r11,_UC_REGS_PTR(r3)
  stw r6,_UC_GREGS+(PT_R3*4)(r11)
  stw r7,_UC_GREGS+(PT_R4*4)(r11)
  stw r8,_UC_GREGS+(PT_R5*4)(r11)
diff -urN libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S
--- libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S 2005-12-29 15:03:38.000000000 -0600
+++ libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext.S 2006-12-13 17:16:39.544244032 -0600
@@ -1,5 +1,5 @@
 /* Jump to a new context.
-   Copyright (C) 2002, 2004, 2005 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -20,6 +20,7 @@
 #include <sysdep.h>
 #include <rtld-global-offsets.h>
 #include <shlib-compat.h>
+#include <kernel-features.h>
 
 #define __ASSEMBLY__
 #include <asm/ptrace.h>
diff -urN libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext.S libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext.S
--- libc25-cvstip-20061129/sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext.S 2005-12-29 15:04:28.000000000 -0600
+++ libc24/sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext.S 2006-12-13 17:11:34.938224360 -0600
@@ -1,5 +1,5 @@
 /* Save current context and jump to a new context.
-   Copyright (C) 2002, 2004, 2005 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -36,6 +36,10 @@
 #ifdef __ASSUME_SWAPCONTEXT_SYSCALL
  .section ".text";
 ENTRY (__swapcontext)
+/* Insure that the _UC_REGS starts on a quadword boundary.  */
+ addi r0,r3,_UC_REG_SPACE+12
+ clrrwi  r0,r0,4
+ stw r0,_UC_REGS_PTR(r3)
  li r5,_UC_SIZE_2_3_4;
  DO_CALL (SYS_ify (swapcontext));
  bso- cr0,1f
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
In reply to this post by Steven Munroe
Steven Munroe writes:

> >Note that sys_swapcontext can happily use, as its second argument, a
> >ucontext where the uc_mcontext.uc_regs field is not 16-byte aligned,
> >though, so glibc could work around the kernel bug by doing the memmove
> >in makecontext as you suggest.
> >
> >  
> I would like to avoid any memmove by aligning the regs buffer properly
> in the first place.

I don't understand why makecontext has to set the regs pointer at all,
given that the user has to call getcontext first.  Why can't
makecontext just use the pointer that getcontext has set?

If you do that then there is no problem using the sys_swapcontext
syscall even on kernels that have the alignment bug, as long as you
use sys_swapcontext for setcontext and swapcontext.  (And yes I will
fix the alignment bug in the kernel.)

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Jakub Jelinek
On Tue, Dec 19, 2006 at 04:48:09PM +1100, Paul Mackerras wrote:

> Steven Munroe writes:
>
> > >Note that sys_swapcontext can happily use, as its second argument, a
> > >ucontext where the uc_mcontext.uc_regs field is not 16-byte aligned,
> > >though, so glibc could work around the kernel bug by doing the memmove
> > >in makecontext as you suggest.
> > >
> > >  
> > I would like to avoid any memmove by aligning the regs buffer properly
> > in the first place.
>
> I don't understand why makecontext has to set the regs pointer at all,
> given that the user has to call getcontext first.  Why can't
> makecontext just use the pointer that getcontext has set?
>
> If you do that then there is no problem using the sys_swapcontext
> syscall even on kernels that have the alignment bug, as long as you
> use sys_swapcontext for setcontext and swapcontext.  (And yes I will
> fix the alignment bug in the kernel.)
I completely agree, I also see no reason to align in
makecontext@@GLIBC_2.3.4.  POSIX requires that the ucontext_t passed
to makecontext has been initialized by a getcontext call, and either
glibc is configured to use swapcontext syscall (in this case
the kernel should make sure it is aligned, but even if it does not,
we are using swapcontext syscall everywhere and the kernel doesn't
need it aligned), or we are not using swapcontext syscall anywhere
and getcontext@@GLIBC_2.3.4 pure userland implementation initializes
uc_mcontext.uc_regs to an aligned value.

Attached is a patch to change makecontext.c as well as a testcase
I posted just inline when starting this thread.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Steven Munroe
Jakub Jelinek wrote:

>On Tue, Dec 19, 2006 at 04:48:09PM +1100, Paul Mackerras wrote:
>  
>
>>Steven Munroe writes:
>>
>>    
>>
>>>>Note that sys_swapcontext can happily use, as its second argument, a
>>>>ucontext where the uc_mcontext.uc_regs field is not 16-byte aligned,
>>>>though, so glibc could work around the kernel bug by doing the memmove
>>>>in makecontext as you suggest.
>>>>
>>>>
>>>>        
>>>>
>>>I would like to avoid any memmove by aligning the regs buffer properly
>>>in the first place.
>>>      
>>>
>>I don't understand why makecontext has to set the regs pointer at all,
>>given that the user has to call getcontext first.  Why can't
>>makecontext just use the pointer that getcontext has set?
>>
>>If you do that then there is no problem using the sys_swapcontext
>>syscall even on kernels that have the alignment bug, as long as you
>>use sys_swapcontext for setcontext and swapcontext.  (And yes I will
>>fix the alignment bug in the kernel.)
>>    
>>
>
>I completely agree, I also see no reason to align in
>makecontext@@GLIBC_2.3.4.  POSIX requires that the ucontext_t passed
>to makecontext has been initialized by a getcontext call, and either
>glibc is configured to use swapcontext syscall (in this case
>the kernel should make sure it is aligned, but even if it does not,
>we are using swapcontext syscall everywhere and the kernel doesn't
>need it aligned), or we are not using swapcontext syscall anywhere
>and getcontext@@GLIBC_2.3.4 pure userland implementation initializes
>uc_mcontext.uc_regs to an aligned value.
>
>Attached is a patch to change makecontext.c as well as a testcase
>I posted just inline when starting this thread.
>
>  
>
This is similar to the patch I submitted on 12/13/06,
http://sources.redhat.com/ml/libc-alpha/2006-12/msg00115.html.

This is fine if all cases that create/initialize a ucontext set the
pointer and align the reg save area. At the moment the makecontext only
sets up integer/pointer parms to pass to the func. But to be complete it
really should handle floating point and vector parms, which requires
that the regs save area is quadword aligned (even if the kernel can
handle unaligned VRs that ABI for parameter passing does not.

My patch went further and aligned and set the uc_regs pointer before the
call to sys_swapcontext (assuming the kernel would use the uc_regs
address). Alternatively the kernel can make sure the reg save area is
aligned and uc_regs is set.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Jakub Jelinek
On Tue, Dec 19, 2006 at 11:15:24AM -0600, Steven Munroe wrote:
> This is similar to the patch I submitted on 12/13/06,
> http://sources.redhat.com/ml/libc-alpha/2006-12/msg00115.html.

I don't understand the getcontext/swapcontext bits in the patch.
There is no need to touch oldctx->uc_mcontext.uc_regs when
oldctx is immediately passed as first argument to swapcontext(2),
that syscall overwrites it anyway:

long sys_swapcontext(struct ucontext __user *old_ctx,
                     struct ucontext __user *new_ctx,
                     int ctx_size, int r6, int r7, int r8, struct pt_regs
                     *regs)
{
        unsigned char tmp;

        /* Context size is for future use. Right now, we only make sure
         * we are passed something we understand
         */
        if (ctx_size < sizeof(struct ucontext))
                return -EINVAL;

        if (old_ctx != NULL) {
                if (!access_ok(VERIFY_WRITE, old_ctx, sizeof(*old_ctx))
                    || save_user_regs(regs, &old_ctx->uc_mcontext, 0)
                    || put_sigset_t(&old_ctx->uc_sigmask, &current->blocked)
                    || __put_user(to_user_ptr(&old_ctx->uc_mcontext),
                            &old_ctx->uc_regs))
                        return -EFAULT;
        }
...

Even when Paul fixes kernel to align it, it IMHO still needs not to look
at the previous old_ctx->uc_regs value to be backwards compatible with
current glibcs which didn't initialize it in any way.

> This is fine if all cases that create/initialize a ucontext set the
> pointer and align the reg save area. At the moment the makecontext only
> sets up integer/pointer parms to pass to the func. But to be complete it
> really should handle floating point and vector parms, which requires
> that the regs save area is quadword aligned (even if the kernel can
> handle unaligned VRs that ABI for parameter passing does not.

??  makecontext arguments are just ints, I don't think we should invent
new makecontext like APIs when makecontext is obsolescent in current POSIX.
So IMHO we should not touch floating point nor altivec regs in makecontext,
they should be inherited from getcontext call.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Steven Munroe
Jakub Jelinek wrote:

>On Tue, Dec 19, 2006 at 11:15:24AM -0600, Steven Munroe wrote:
>  
>
>>This is similar to the patch I submitted on 12/13/06,
>>http://sources.redhat.com/ml/libc-alpha/2006-12/msg00115.html.
>>    
>>
>
>I don't understand the getcontext/swapcontext bits in the patch.
>There is no need to touch oldctx->uc_mcontext.uc_regs when
>oldctx is immediately passed as first argument to swapcontext(2),
>that syscall overwrites it anyway:
>  
>
If that is true for old versions of the kernels as well?

If all versions of kernel where __ASSUME_SWAPCONTEXT_SYSCALL is defined,
sets the uc_regs pointer to at least double word then just the
makecontext change is required.

I was just being careful.
Reply | Threaded
Open this post in threaded view
|

Re: {make,set,swap}context broken on powerpc32

Paul Mackerras
Steven Munroe writes:

> >I don't understand the getcontext/swapcontext bits in the patch.
> >There is no need to touch oldctx->uc_mcontext.uc_regs when
> >oldctx is immediately passed as first argument to swapcontext(2),
> >that syscall overwrites it anyway:
> >  
> >
> If that is true for old versions of the kernels as well?

All kernels that have a swapcontext system call set
oldctx->uc_mcontext.uc_regs (assuming oldctx != NULL).

> If all versions of kernel where __ASSUME_SWAPCONTEXT_SYSCALL is defined,
> sets the uc_regs pointer to at least double word then just the
> makecontext change is required.

The alignment of uc_regs will be the same as that of old_ctx, which is
probably only 4 bytes.  However, that's good enough for loading and
storing GPRs and FPRs.

Paul.