| Message ID | cf79b9f0c40314e6bfda7c634e378015bd7ba037.1426180120.git.josh@joshtriplett.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: > For 32-bit userspace on a 64-bit kernel, this requires modifying > stub32_clone to actually swap the appropriate arguments to match > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls > broken. > > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> > --- > arch/x86/Kconfig | 1 + > arch/x86/ia32/ia32entry.S | 2 +- > arch/x86/kernel/process_32.c | 6 +++--- > arch/x86/kernel/process_64.c | 8 ++++---- > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index b7d31ca..4960b0d 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -124,6 +124,7 @@ config X86 > select MODULES_USE_ELF_REL if X86_32 > select MODULES_USE_ELF_RELA if X86_64 > select CLONE_BACKWARDS if X86_32 > + select HAVE_COPY_THREAD_TLS > select ARCH_USE_BUILTIN_BSWAP > select ARCH_USE_QUEUE_RWLOCK > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S > index 156ebca..0286735 100644 > --- a/arch/x86/ia32/ia32entry.S > +++ b/arch/x86/ia32/ia32entry.S > @@ -487,7 +487,7 @@ GLOBAL(\label) > ALIGN > GLOBAL(stub32_clone) > leaq sys_clone(%rip),%rax > - mov %r8, %rcx > + xchg %r8, %rcx > jmp ia32_ptregs_common Do I understand correct that whatever function this is a stub for just takes its arguments in the wrong order? If so, can we just fix it instead of using xchg here? In general, I much prefer C code to new asm where it makes sense to make this tradeoff. Other than that, this is a huge improvement. You'll have minor conflicts against -tip, though. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote: > On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: > > For 32-bit userspace on a 64-bit kernel, this requires modifying > > stub32_clone to actually swap the appropriate arguments to match > > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls > > broken. > > > > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> > > --- > > arch/x86/Kconfig | 1 + > > arch/x86/ia32/ia32entry.S | 2 +- > > arch/x86/kernel/process_32.c | 6 +++--- > > arch/x86/kernel/process_64.c | 8 ++++---- > > 4 files changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index b7d31ca..4960b0d 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -124,6 +124,7 @@ config X86 > > select MODULES_USE_ELF_REL if X86_32 > > select MODULES_USE_ELF_RELA if X86_64 > > select CLONE_BACKWARDS if X86_32 > > + select HAVE_COPY_THREAD_TLS > > select ARCH_USE_BUILTIN_BSWAP > > select ARCH_USE_QUEUE_RWLOCK > > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION > > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S > > index 156ebca..0286735 100644 > > --- a/arch/x86/ia32/ia32entry.S > > +++ b/arch/x86/ia32/ia32entry.S > > @@ -487,7 +487,7 @@ GLOBAL(\label) > > ALIGN > > GLOBAL(stub32_clone) > > leaq sys_clone(%rip),%rax > > - mov %r8, %rcx > > + xchg %r8, %rcx > > jmp ia32_ptregs_common > > Do I understand correct that whatever function this is a stub for just > takes its arguments in the wrong order? If so, can we just fix it > instead of using xchg here? 32-bit x86 and 64-bit x86 take the arguments to clone in a different order, and stub32_clone fixes up the argument order then calls the 64-bit sys_clone. I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C under CONFIG_COMPAT. However, doing so would require encoding the knowledge for each 64-bit architecture for how its corresponding 32-bit architecture accepts arguments to clone, which is information that the current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then require cleaning up all the architecture-specific assembly stubs for 32-bit clone entry points. In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't seem worth it, since it would require adding a new C entry point for compat_sys_clone under arch/x86 somewhere. One cleanup at a time. :) > In general, I much prefer C code to new asm where it makes sense to > make this tradeoff. Agreed completely. However, this is at least conservation-of-asm, or reduction if you consider the pt_regs argument-grabbing hack to be asm-esque code. > Other than that, this is a huge improvement. You'll have minor > conflicts against -tip, though. Right, I've seen your current changes there. Should be a trivial merge though. Would you mind providing an ack for the series, or at least for the first two patches? (I'm wondering whose tree this series ought to go through, for that matter.) - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 3:31 PM, <josh@joshtriplett.org> wrote: > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote: >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: >> > For 32-bit userspace on a 64-bit kernel, this requires modifying >> > stub32_clone to actually swap the appropriate arguments to match >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls >> > broken. >> > >> > Signed-off-by: Josh Triplett <josh@joshtriplett.org> >> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> >> > --- >> > arch/x86/Kconfig | 1 + >> > arch/x86/ia32/ia32entry.S | 2 +- >> > arch/x86/kernel/process_32.c | 6 +++--- >> > arch/x86/kernel/process_64.c | 8 ++++---- >> > 4 files changed, 9 insertions(+), 8 deletions(-) >> > >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> > index b7d31ca..4960b0d 100644 >> > --- a/arch/x86/Kconfig >> > +++ b/arch/x86/Kconfig >> > @@ -124,6 +124,7 @@ config X86 >> > select MODULES_USE_ELF_REL if X86_32 >> > select MODULES_USE_ELF_RELA if X86_64 >> > select CLONE_BACKWARDS if X86_32 >> > + select HAVE_COPY_THREAD_TLS >> > select ARCH_USE_BUILTIN_BSWAP >> > select ARCH_USE_QUEUE_RWLOCK >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S >> > index 156ebca..0286735 100644 >> > --- a/arch/x86/ia32/ia32entry.S >> > +++ b/arch/x86/ia32/ia32entry.S >> > @@ -487,7 +487,7 @@ GLOBAL(\label) >> > ALIGN >> > GLOBAL(stub32_clone) >> > leaq sys_clone(%rip),%rax >> > - mov %r8, %rcx >> > + xchg %r8, %rcx >> > jmp ia32_ptregs_common >> >> Do I understand correct that whatever function this is a stub for just >> takes its arguments in the wrong order? If so, can we just fix it >> instead of using xchg here? > > 32-bit x86 and 64-bit x86 take the arguments to clone in a different > order, and stub32_clone fixes up the argument order then calls the > 64-bit sys_clone. > > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C > under CONFIG_COMPAT. However, doing so would require encoding the > knowledge for each 64-bit architecture for how its corresponding 32-bit > architecture accepts arguments to clone, which is information that the > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then > require cleaning up all the architecture-specific assembly stubs for > 32-bit clone entry points. > > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't > seem worth it, since it would require adding a new C entry point for > compat_sys_clone under arch/x86 somewhere. > > One cleanup at a time. :) Fine w/ me. > >> In general, I much prefer C code to new asm where it makes sense to >> make this tradeoff. > > Agreed completely. However, this is at least conservation-of-asm, or > reduction if you consider the pt_regs argument-grabbing hack to be > asm-esque code. > >> Other than that, this is a huge improvement. You'll have minor >> conflicts against -tip, though. > > Right, I've seen your current changes there. Should be a trivial merge > though. > > Would you mind providing an ack for the series, or at least for the > first two patches? I can give you an ok-in-principle on the first two. I'd need to stare at the awful code for a bit to understand the @!*&! clone variants to really ack them convincingly. OTOH, it would be rather surprising if you messed it up in a way that still boots on all three variants (native 32-bit, native 64-bit, and compat). So, for the first two patches: Acked-by: Andy Lutomirski <luto@kernel.org> # assuming all bitnesses boot --Andy > > (I'm wondering whose tree this series ought to go through, for that > matter.) > > - Josh Triplett
On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote: > On Fri, Mar 13, 2015 at 3:31 PM, <josh@joshtriplett.org> wrote: > > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote: > >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >> > For 32-bit userspace on a 64-bit kernel, this requires modifying > >> > stub32_clone to actually swap the appropriate arguments to match > >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls > >> > broken. > >> > > >> > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > >> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> > >> > --- > >> > arch/x86/Kconfig | 1 + > >> > arch/x86/ia32/ia32entry.S | 2 +- > >> > arch/x86/kernel/process_32.c | 6 +++--- > >> > arch/x86/kernel/process_64.c | 8 ++++---- > >> > 4 files changed, 9 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > >> > index b7d31ca..4960b0d 100644 > >> > --- a/arch/x86/Kconfig > >> > +++ b/arch/x86/Kconfig > >> > @@ -124,6 +124,7 @@ config X86 > >> > select MODULES_USE_ELF_REL if X86_32 > >> > select MODULES_USE_ELF_RELA if X86_64 > >> > select CLONE_BACKWARDS if X86_32 > >> > + select HAVE_COPY_THREAD_TLS > >> > select ARCH_USE_BUILTIN_BSWAP > >> > select ARCH_USE_QUEUE_RWLOCK > >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION > >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S > >> > index 156ebca..0286735 100644 > >> > --- a/arch/x86/ia32/ia32entry.S > >> > +++ b/arch/x86/ia32/ia32entry.S > >> > @@ -487,7 +487,7 @@ GLOBAL(\label) > >> > ALIGN > >> > GLOBAL(stub32_clone) > >> > leaq sys_clone(%rip),%rax > >> > - mov %r8, %rcx > >> > + xchg %r8, %rcx > >> > jmp ia32_ptregs_common > >> > >> Do I understand correct that whatever function this is a stub for just > >> takes its arguments in the wrong order? If so, can we just fix it > >> instead of using xchg here? > > > > 32-bit x86 and 64-bit x86 take the arguments to clone in a different > > order, and stub32_clone fixes up the argument order then calls the > > 64-bit sys_clone. > > > > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C > > under CONFIG_COMPAT. However, doing so would require encoding the > > knowledge for each 64-bit architecture for how its corresponding 32-bit > > architecture accepts arguments to clone, which is information that the > > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then > > require cleaning up all the architecture-specific assembly stubs for > > 32-bit clone entry points. > > > > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't > > seem worth it, since it would require adding a new C entry point for > > compat_sys_clone under arch/x86 somewhere. > > > > One cleanup at a time. :) > > Fine w/ me. Thanks. > > > >> In general, I much prefer C code to new asm where it makes sense to > >> make this tradeoff. > > > > Agreed completely. However, this is at least conservation-of-asm, or > > reduction if you consider the pt_regs argument-grabbing hack to be > > asm-esque code. > > > >> Other than that, this is a huge improvement. You'll have minor > >> conflicts against -tip, though. > > > > Right, I've seen your current changes there. Should be a trivial merge > > though. > > > > Would you mind providing an ack for the series, or at least for the > > first two patches? > > I can give you an ok-in-principle on the first two. I'd need to stare > at the awful code for a bit to understand the @!*&! clone variants to > really ack them convincingly. I'd definitely appreciate the staring. :) > OTOH, it would be rather surprising if you messed it up in a way that > still boots on all three variants (native 32-bit, native 64-bit, and > compat). > > So, for the first two patches: > > Acked-by: Andy Lutomirski <luto@kernel.org> # assuming all bitnesses boot I did test all three, not just with booting but with a thread-local storage test. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 3:43 PM, <josh@joshtriplett.org> wrote: > On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote: >> On Fri, Mar 13, 2015 at 3:31 PM, <josh@joshtriplett.org> wrote: >> > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote: >> >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: >> >> > For 32-bit userspace on a 64-bit kernel, this requires modifying >> >> > stub32_clone to actually swap the appropriate arguments to match >> >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls >> >> > broken. >> >> > >> >> > Signed-off-by: Josh Triplett <josh@joshtriplett.org> >> >> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> >> >> > --- >> >> > arch/x86/Kconfig | 1 + >> >> > arch/x86/ia32/ia32entry.S | 2 +- >> >> > arch/x86/kernel/process_32.c | 6 +++--- >> >> > arch/x86/kernel/process_64.c | 8 ++++---- >> >> > 4 files changed, 9 insertions(+), 8 deletions(-) >> >> > >> >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> >> > index b7d31ca..4960b0d 100644 >> >> > --- a/arch/x86/Kconfig >> >> > +++ b/arch/x86/Kconfig >> >> > @@ -124,6 +124,7 @@ config X86 >> >> > select MODULES_USE_ELF_REL if X86_32 >> >> > select MODULES_USE_ELF_RELA if X86_64 >> >> > select CLONE_BACKWARDS if X86_32 >> >> > + select HAVE_COPY_THREAD_TLS >> >> > select ARCH_USE_BUILTIN_BSWAP >> >> > select ARCH_USE_QUEUE_RWLOCK >> >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION >> >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S >> >> > index 156ebca..0286735 100644 >> >> > --- a/arch/x86/ia32/ia32entry.S >> >> > +++ b/arch/x86/ia32/ia32entry.S >> >> > @@ -487,7 +487,7 @@ GLOBAL(\label) >> >> > ALIGN >> >> > GLOBAL(stub32_clone) >> >> > leaq sys_clone(%rip),%rax >> >> > - mov %r8, %rcx >> >> > + xchg %r8, %rcx >> >> > jmp ia32_ptregs_common >> >> >> >> Do I understand correct that whatever function this is a stub for just >> >> takes its arguments in the wrong order? If so, can we just fix it >> >> instead of using xchg here? >> > >> > 32-bit x86 and 64-bit x86 take the arguments to clone in a different >> > order, and stub32_clone fixes up the argument order then calls the >> > 64-bit sys_clone. >> > >> > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C >> > under CONFIG_COMPAT. However, doing so would require encoding the >> > knowledge for each 64-bit architecture for how its corresponding 32-bit >> > architecture accepts arguments to clone, which is information that the >> > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then >> > require cleaning up all the architecture-specific assembly stubs for >> > 32-bit clone entry points. >> > >> > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't >> > seem worth it, since it would require adding a new C entry point for >> > compat_sys_clone under arch/x86 somewhere. >> > >> > One cleanup at a time. :) >> >> Fine w/ me. > > Thanks. > >> > >> >> In general, I much prefer C code to new asm where it makes sense to >> >> make this tradeoff. >> > >> > Agreed completely. However, this is at least conservation-of-asm, or >> > reduction if you consider the pt_regs argument-grabbing hack to be >> > asm-esque code. >> > >> >> Other than that, this is a huge improvement. You'll have minor >> >> conflicts against -tip, though. >> > >> > Right, I've seen your current changes there. Should be a trivial merge >> > though. >> > >> > Would you mind providing an ack for the series, or at least for the >> > first two patches? >> >> I can give you an ok-in-principle on the first two. I'd need to stare >> at the awful code for a bit to understand the @!*&! clone variants to >> really ack them convincingly. > > I'd definitely appreciate the staring. :) > >> OTOH, it would be rather surprising if you messed it up in a way that >> still boots on all three variants (native 32-bit, native 64-bit, and >> compat). >> >> So, for the first two patches: >> >> Acked-by: Andy Lutomirski <luto@kernel.org> # assuming all bitnesses boot > > I did test all three, not just with booting but with a thread-local > storage test. And it's fairly clear that no one ever tested clone-based TLS in 32 bits from a 64-bit ELF binary, because it was broken until very recently :-/ This stuff is too magical and too poorly documented for my tastes. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 03:45:16PM -0700, Andy Lutomirski wrote: > On Fri, Mar 13, 2015 at 3:43 PM, <josh@joshtriplett.org> wrote: > > On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote: > >> On Fri, Mar 13, 2015 at 3:31 PM, <josh@joshtriplett.org> wrote: > >> > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote: > >> >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >> >> > For 32-bit userspace on a 64-bit kernel, this requires modifying > >> >> > stub32_clone to actually swap the appropriate arguments to match > >> >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls > >> >> > broken. > >> >> > > >> >> > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > >> >> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com> > >> >> > --- > >> >> > arch/x86/Kconfig | 1 + > >> >> > arch/x86/ia32/ia32entry.S | 2 +- > >> >> > arch/x86/kernel/process_32.c | 6 +++--- > >> >> > arch/x86/kernel/process_64.c | 8 ++++---- > >> >> > 4 files changed, 9 insertions(+), 8 deletions(-) > >> >> > > >> >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > >> >> > index b7d31ca..4960b0d 100644 > >> >> > --- a/arch/x86/Kconfig > >> >> > +++ b/arch/x86/Kconfig > >> >> > @@ -124,6 +124,7 @@ config X86 > >> >> > select MODULES_USE_ELF_REL if X86_32 > >> >> > select MODULES_USE_ELF_RELA if X86_64 > >> >> > select CLONE_BACKWARDS if X86_32 > >> >> > + select HAVE_COPY_THREAD_TLS > >> >> > select ARCH_USE_BUILTIN_BSWAP > >> >> > select ARCH_USE_QUEUE_RWLOCK > >> >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION > >> >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S > >> >> > index 156ebca..0286735 100644 > >> >> > --- a/arch/x86/ia32/ia32entry.S > >> >> > +++ b/arch/x86/ia32/ia32entry.S > >> >> > @@ -487,7 +487,7 @@ GLOBAL(\label) > >> >> > ALIGN > >> >> > GLOBAL(stub32_clone) > >> >> > leaq sys_clone(%rip),%rax > >> >> > - mov %r8, %rcx > >> >> > + xchg %r8, %rcx > >> >> > jmp ia32_ptregs_common > >> >> > >> >> Do I understand correct that whatever function this is a stub for just > >> >> takes its arguments in the wrong order? If so, can we just fix it > >> >> instead of using xchg here? > >> > > >> > 32-bit x86 and 64-bit x86 take the arguments to clone in a different > >> > order, and stub32_clone fixes up the argument order then calls the > >> > 64-bit sys_clone. > >> > > >> > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C > >> > under CONFIG_COMPAT. However, doing so would require encoding the > >> > knowledge for each 64-bit architecture for how its corresponding 32-bit > >> > architecture accepts arguments to clone, which is information that the > >> > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then > >> > require cleaning up all the architecture-specific assembly stubs for > >> > 32-bit clone entry points. > >> > > >> > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't > >> > seem worth it, since it would require adding a new C entry point for > >> > compat_sys_clone under arch/x86 somewhere. > >> > > >> > One cleanup at a time. :) > >> > >> Fine w/ me. > > > > Thanks. > > > >> > > >> >> In general, I much prefer C code to new asm where it makes sense to > >> >> make this tradeoff. > >> > > >> > Agreed completely. However, this is at least conservation-of-asm, or > >> > reduction if you consider the pt_regs argument-grabbing hack to be > >> > asm-esque code. > >> > > >> >> Other than that, this is a huge improvement. You'll have minor > >> >> conflicts against -tip, though. > >> > > >> > Right, I've seen your current changes there. Should be a trivial merge > >> > though. > >> > > >> > Would you mind providing an ack for the series, or at least for the > >> > first two patches? > >> > >> I can give you an ok-in-principle on the first two. I'd need to stare > >> at the awful code for a bit to understand the @!*&! clone variants to > >> really ack them convincingly. > > > > I'd definitely appreciate the staring. :) > > > >> OTOH, it would be rather surprising if you messed it up in a way that > >> still boots on all three variants (native 32-bit, native 64-bit, and > >> compat). > >> > >> So, for the first two patches: > >> > >> Acked-by: Andy Lutomirski <luto@kernel.org> # assuming all bitnesses boot > > > > I did test all three, not just with booting but with a thread-local > > storage test. > > And it's fairly clear that no one ever tested clone-based TLS in 32 > bits from a 64-bit ELF binary, because it was broken until very > recently :-/ I'm not sure *anyone* other than exploit-seekers test 32-bit system calls from a 64-bit binary. :) > This stuff is too magical and too poorly documented for my tastes. Agreed. That was my reaction when I figured out what was happening with CLONE_SETTLS and pt_regs, and my goal with the first two patches in this series was precisely to make it *less* magical. :) - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index b7d31ca..4960b0d 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -124,6 +124,7 @@ config X86 select MODULES_USE_ELF_REL if X86_32 select MODULES_USE_ELF_RELA if X86_64 select CLONE_BACKWARDS if X86_32 + select HAVE_COPY_THREAD_TLS select ARCH_USE_BUILTIN_BSWAP select ARCH_USE_QUEUE_RWLOCK select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S index 156ebca..0286735 100644 --- a/arch/x86/ia32/ia32entry.S +++ b/arch/x86/ia32/ia32entry.S @@ -487,7 +487,7 @@ GLOBAL(\label) ALIGN GLOBAL(stub32_clone) leaq sys_clone(%rip),%rax - mov %r8, %rcx + xchg %r8, %rcx jmp ia32_ptregs_common ALIGN diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index 603c4f9..ead28ff 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -129,8 +129,8 @@ void release_thread(struct task_struct *dead_task) release_vm86_irqs(dead_task); } -int copy_thread(unsigned long clone_flags, unsigned long sp, - unsigned long arg, struct task_struct *p) +int copy_thread_tls(unsigned long clone_flags, unsigned long sp, + unsigned long arg, struct task_struct *p, unsigned long tls) { struct pt_regs *childregs = task_pt_regs(p); struct task_struct *tsk; @@ -185,7 +185,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, */ if (clone_flags & CLONE_SETTLS) err = do_set_thread_area(p, -1, - (struct user_desc __user *)childregs->si, 0); + (struct user_desc __user *)tls, 0); if (err && p->thread.io_bitmap_ptr) { kfree(p->thread.io_bitmap_ptr); diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 67fcc43..c69cabc 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -151,8 +151,8 @@ static inline u32 read_32bit_tls(struct task_struct *t, int tls) return get_desc_base(&t->thread.tls_array[tls]); } -int copy_thread(unsigned long clone_flags, unsigned long sp, - unsigned long arg, struct task_struct *p) +int copy_thread_tls(unsigned long clone_flags, unsigned long sp, + unsigned long arg, struct task_struct *p, unsigned long tls) { int err; struct pt_regs *childregs; @@ -209,10 +209,10 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, #ifdef CONFIG_IA32_EMULATION if (test_thread_flag(TIF_IA32)) err = do_set_thread_area(p, -1, - (struct user_desc __user *)childregs->si, 0); + (struct user_desc __user *)tls, 0); else #endif - err = do_arch_prctl(p, ARCH_SET_FS, childregs->r8); + err = do_arch_prctl(p, ARCH_SET_FS, tls); if (err) goto out; }