Skip to contentSkip to conversation

x86_64: Sigcontext improvements

PATCH In review Andy Lutomirski 4 participants 15 messages Mar 10, 2015 – Mar 12, 2015 x86

lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

v2 cover x86_64: Sigcontext improvements

Patch 1 is IMO a bug fix.

Patch 2 is a cleanup and avoids some confusion. It's also sort of an API break (no ABI change) because it removes a struct field. But that struct field has never done anything at all as far as I can tell.

AFAICT this series got bogged down in discussion about patch 2 last time. I'm only including patch 2 for completeness here -- patch 1 is IMO far more useful, and I'm fine with patch 1 being merged and tabling patch 2 again.

Changes from v1:
- Add the __USER_DS fix, caught by my sigreturn test.
- Re-tested locally.

Andy Lutomirski (2):
x86_64,signal: Fix SS handling for signals delivered to 64-bit
programs
x86_64,signal: Remove 'fs' and 'gs' from sigcontext

 arch/x86/include/asm/sigcontext.h      |  6 +++---
 arch/x86/include/uapi/asm/sigcontext.h |  6 +++---
 arch/x86/kernel/signal.c               | 24 +++++++++++++-----------
 3 files changed, 19 insertions(+), 17 deletions(-)
Signature · 1 line
2.3.0
author
#1 lore raw

PATCH v2 1/2 x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

I suspect that 64-bit programs that try to run 16-bit code and uses signals will have a lot of trouble without this.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)
arch/x86/include/asm/sigcontext.h +1 -1
index 9dfce4e0417d..f910cdcb71fd 100644
@@ -59,7 +59,7 @@ struct sigcontext {
 	unsigned short cs;
 	unsigned short gs;
 	unsigned short fs;
-	unsigned short __pad0;
+	unsigned short ss;
 	unsigned long err;
 	unsigned long trapno;
 	unsigned long oldmask;
arch/x86/include/uapi/asm/sigcontext.h +1 -1
index d8b9f9081e86..076b11fd6fa1 100644
@@ -179,7 +179,7 @@ struct sigcontext {
 	__u16 cs;
 	__u16 gs;
 	__u16 fs;
-	__u16 __pad0;
+	__u16 ss;
 	__u64 err;
 	__u64 trapno;
 	__u64 oldmask;
arch/x86/kernel/signal.c +11 -9
index ed37a768d0fc..40f34574fb36 100644
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 		COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
 		COPY_SEG_CPL3(cs);
 		COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-		/* Kernel saves and restores only the CS segment register on signals,
-		 * which is the bare minimum needed to allow mixed 32/64-bit code.
-		 * App's signal handler can save/restore other segments if needed. */
-		COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
 		get_user_ex(tmpflags, &sc->flags);
 		regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);

@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 		put_user_ex(regs->cs, &sc->cs);
 		put_user_ex(0, &sc->gs);
 		put_user_ex(0, &sc->fs);
+		put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
 		put_user_ex(fpstate, &sc->fpstate);

@@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
 	regs->sp = (unsigned long)frame;
 
-	/* Set up the CS register to run signal handlers in 64-bit mode,
-	   even if the handler happens to be interrupting 32-bit code. */
+	/* Set up the CS and SS registers to run signal handlers in
+	   64-bit mode, even if the handler happens to be interrupting
+	   32-bit or 16-bit code.
+
+	   SS is subtle.  In 64-bit mode, we don't need any particular
+	   SS descriptor, but we do need SS to be valid.  It's possible
+	   that the old SS is entirely bogus -- this can happen if the
+	   signal we're trying to deliver is #GP or #SS caused by a bad
+	   SS value. */
 	regs->cs = __USER_CS;
+	regs->ss = __USER_DS;
 
 	return 0;
 }
Signature · 1 line
2.3.0
author
#2 lore raw

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[cc: Oleg, Borislav]

Quoted 101 lines · Andy Lutomirski

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

I suspect that 64-bit programs that try to run 16-bit code and uses signals will have a lot of trouble without this.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)
arch/x86/include/asm/sigcontext.h +1 -1
index 9dfce4e0417d..f910cdcb71fd 100644
@@ -59,7 +59,7 @@ struct sigcontext {
        unsigned short cs;
        unsigned short gs;
        unsigned short fs;
-       unsigned short __pad0;
+       unsigned short ss;
        unsigned long err;
        unsigned long trapno;
        unsigned long oldmask;
arch/x86/include/uapi/asm/sigcontext.h +1 -1
index d8b9f9081e86..076b11fd6fa1 100644
@@ -179,7 +179,7 @@ struct sigcontext {
        __u16 cs;
        __u16 gs;
        __u16 fs;
-       __u16 __pad0;
+       __u16 ss;
        __u64 err;
        __u64 trapno;
        __u64 oldmask;
arch/x86/kernel/signal.c +11 -10
index ed37a768d0fc..40f34574fb36 100644
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
                COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
                COPY_SEG_CPL3(cs);
                COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-               /* Kernel saves and restores only the CS segment register on signals,
-                * which is the bare minimum needed to allow mixed 32/64-bit code.
-                * App's signal handler can save/restore other segments if needed. */
-               COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
                get_user_ex(tmpflags, &sc->flags);
                regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);

@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
                put_user_ex(regs->cs, &sc->cs);
                put_user_ex(0, &sc->gs);
                put_user_ex(0, &sc->fs);
+               put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
                put_user_ex(fpstate, &sc->fpstate);

@@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
        regs->sp = (unsigned long)frame;
 
-       /* Set up the CS register to run signal handlers in 64-bit mode,
-          even if the handler happens to be interrupting 32-bit code. */
+       /* Set up the CS and SS registers to run signal handlers in
+          64-bit mode, even if the handler happens to be interrupting
+          32-bit or 16-bit code.
+
+          SS is subtle.  In 64-bit mode, we don't need any particular
+          SS descriptor, but we do need SS to be valid.  It's possible
+          that the old SS is entirely bogus -- this can happen if the
+          signal we're trying to deliver is #GP or #SS caused by a bad
+          SS value. */
        regs->cs = __USER_CS;
+       regs->ss = __USER_DS;
 
        return 0;
 }
--

2.3.0

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#3 lore raw

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

Well, the patch looks "obviously fine" to me, but this is all I can say.

I mean, I simply can't understand this __pad0/ifdef(CONFIG_X86_32), it looks as if ->ss was specially excluded for unknown reason from the very beginning.

Quoted 9 lines · Andy Lutomirski

arch/x86/include/uapi/asm/sigcontext.h struct sigcontext { · line 182

Quoted 9 lines
arch/x86/include/uapi/asm/sigcontext.h +1 -1
@@ -179,7 +179,7 @@ struct sigcontext {
        __u16 cs;
        __u16 gs;
        __u16 fs;
-       __u16 __pad0;
+       __u16 ss;

I do not know the rules for include/uapi/ ...

OK, nobody should ever use __pad0, so probably it is safe to rename it. OTOH, an application can (say) try to print all members for debugging purposes, it won't compile after this change.

Oleg.

author
#4 lore raw

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

Quoted 5 lines · Oleg Nesterov

Well, the patch looks "obviously fine" to me, but this is all I can say.

I mean, I simply can't understand this __pad0/ifdef(CONFIG_X86_32), it looks as if ->ss was specially excluded for unknown reason from the very beginning.

I assume it was confusion in the very early days of x86_64. Andi?

Quoted 14 lines · Andy Lutomirski
Quoted 9 lines

arch/x86/include/uapi/asm/sigcontext.h struct sigcontext { · line 182

Quoted 9 lines
arch/x86/include/uapi/asm/sigcontext.h +1 -1
@@ -179,7 +179,7 @@ struct sigcontext {
        __u16 cs;
        __u16 gs;
        __u16 fs;
-       __u16 __pad0;
+       __u16 ss;

I do not know the rules for include/uapi/ ...

Quoted 3 lines

OK, nobody should ever use __pad0, so probably it is safe to rename it. OTOH, an application can (say) try to print all members for debugging purposes, it won't compile after this change.

Yeah, I don't really know what the rules are.

--Andy

maintainer
#5 lore raw

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

In reply to Andy Lutomirski
Quoted 6 lines · Andy Lutomirski

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

Quoted 7 lines

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

Are we at least reporting the bad SS value when delivering the signal so that userpsace knows why it got the signal?

Quoted 2 lines

I suspect that 64-bit programs that try to run 16-bit code and uses signals will have a lot of trouble without this.

Do we even have software doing that? Maybe we should search for similar bug reports...

Quoted 12 lines
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)
arch/x86/include/asm/sigcontext.h +0 -0
index 9dfce4e0417d..f910cdcb71fd 100644

arch/x86/include/asm/sigcontext.h struct sigcontext { · line 62

Quoted 4 lines
arch/x86/include/asm/sigcontext.h +0 -1
@@ -59,7 +59,7 @@ struct sigcontext {
 	unsigned short cs;
 	unsigned short gs;
 	unsigned short fs;
-	unsigned short __pad0;

This __pad0 thing has been there since the beginning, according to my git history dive.

Quoted 50 lines
arch/x86/include/asm/sigcontext.h +1 -0
+	unsigned short ss;
 	unsigned long err;
 	unsigned long trapno;
 	unsigned long oldmask;
arch/x86/include/uapi/asm/sigcontext.h +1 -1
index d8b9f9081e86..076b11fd6fa1 100644
@@ -179,7 +179,7 @@ struct sigcontext {
 	__u16 cs;
 	__u16 gs;
 	__u16 fs;
-	__u16 __pad0;
+	__u16 ss;
 	__u64 err;
 	__u64 trapno;
 	__u64 oldmask;
arch/x86/kernel/signal.c +1 -8
index ed37a768d0fc..40f34574fb36 100644
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 		COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
 		COPY_SEG_CPL3(cs);
 		COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-		/* Kernel saves and restores only the CS segment register on signals,
-		 * which is the bare minimum needed to allow mixed 32/64-bit code.
-		 * App's signal handler can save/restore other segments if needed. */
-		COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
 		get_user_ex(tmpflags, &sc->flags);
 		regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);

@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 		put_user_ex(regs->cs, &sc->cs);
 		put_user_ex(0, &sc->gs);
 		put_user_ex(0, &sc->fs);
+		put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
 		put_user_ex(fpstate, &sc->fpstate);

@@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
 	regs->sp = (unsigned long)frame;
 
-	/* Set up the CS register to run signal handlers in 64-bit mode,

arch/x86/kernel/signal.c static int __setup_rt_frame(int sig, struct ksignal *ksig, · line 462

Quoted 10 lines
arch/x86/kernel/signal.c +9 -1
-	   even if the handler happens to be interrupting 32-bit code. */
+	/* Set up the CS and SS registers to run signal handlers in
+	   64-bit mode, even if the handler happens to be interrupting
+	   32-bit or 16-bit code.
+
+	   SS is subtle.  In 64-bit mode, we don't need any particular
+	   SS descriptor, but we do need SS to be valid.  It's possible
+	   that the old SS is entirely bogus -- this can happen if the
+	   signal we're trying to deliver is #GP or #SS caused by a bad
+	   SS value. */

Kernel comment style please:

	/*
	 * Andy likes to go and play 16-bit games on 64-bit linux. We all are
	 * having lotsa fun.
	 */

:-D

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
author
#6 lore raw

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

Quoted 10 lines · Borislav Petkov
Quoted 8 lines · Andy Lutomirski

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

Quoted 8 lines
Quoted 5 lines

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

Are we at least reporting the bad SS value when delivering the signal so that userpsace knows why it got the signal?

Yes, more or less. We'll send sigsegv with an appropriate trapnr. Without this fix, though, we fail to deliver that signal and kill the process.

Quoted 90 lines
Quoted 3 lines

I suspect that 64-bit programs that try to run 16-bit code and uses signals will have a lot of trouble without this.

Do we even have software doing that? Maybe we should search for similar bug reports...

Quoted 12 lines
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)
arch/x86/include/asm/sigcontext.h +0 -0
index 9dfce4e0417d..f910cdcb71fd 100644

arch/x86/include/asm/sigcontext.h struct sigcontext { · line 62

Quoted 4 lines
arch/x86/include/asm/sigcontext.h +0 -1
@@ -59,7 +59,7 @@ struct sigcontext {
      unsigned short cs;
      unsigned short gs;
      unsigned short fs;
-     unsigned short __pad0;

This __pad0 thing has been there since the beginning, according to my git history dive.

Quoted 50 lines
arch/x86/include/asm/sigcontext.h +1 -0
+     unsigned short ss;
      unsigned long err;
      unsigned long trapno;
      unsigned long oldmask;
arch/x86/include/uapi/asm/sigcontext.h +1 -1
index d8b9f9081e86..076b11fd6fa1 100644
@@ -179,7 +179,7 @@ struct sigcontext {
      __u16 cs;
      __u16 gs;
      __u16 fs;
-     __u16 __pad0;
+     __u16 ss;
      __u64 err;
      __u64 trapno;
      __u64 oldmask;
arch/x86/kernel/signal.c +1 -8
index ed37a768d0fc..40f34574fb36 100644
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
              COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
              COPY_SEG_CPL3(cs);
              COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-             /* Kernel saves and restores only the CS segment register on signals,
-              * which is the bare minimum needed to allow mixed 32/64-bit code.
-              * App's signal handler can save/restore other segments if needed. */
-             COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
              get_user_ex(tmpflags, &sc->flags);
              regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);

@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
              put_user_ex(regs->cs, &sc->cs);
              put_user_ex(0, &sc->gs);
              put_user_ex(0, &sc->fs);
+             put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
              put_user_ex(fpstate, &sc->fpstate);

@@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
      regs->sp = (unsigned long)frame;
 
-     /* Set up the CS register to run signal handlers in 64-bit mode,

arch/x86/kernel/signal.c static int __setup_rt_frame(int sig, struct ksignal *ksig, · line 462

Quoted 10 lines
arch/x86/kernel/signal.c +9 -1
-        even if the handler happens to be interrupting 32-bit code. */
+     /* Set up the CS and SS registers to run signal handlers in
+        64-bit mode, even if the handler happens to be interrupting
+        32-bit or 16-bit code.
+
+        SS is subtle.  In 64-bit mode, we don't need any particular
+        SS descriptor, but we do need SS to be valid.  It's possible
+        that the old SS is entirely bogus -- this can happen if the
+        signal we're trying to deliver is #GP or #SS caused by a bad
+        SS value. */

Kernel comment style please:

Quoted 6 lines

/*
* Andy likes to go and play 16-bit games on 64-bit linux. We all are
* having lotsa fun.
*/

:-D

I did it this way for consistency with every other comment in that function. I'll change it.

Quoted 7 lines

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

author
#7 lore raw

PATCH v2 2/2 x86_64,signal: Remove 'fs' and 'gs' from sigcontext

In reply to Andy Lutomirski

As far as I can tell, these fields have been set to zero on save and ignored on restore since Linux was imported into git. Rename them '__pad1' and '__pad2' to avoid confusion and to allow them to be recycled some day.

I'm intentionally avoiding calling either of them __pad0: the field formerly known as __pad0 is now ss.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      | 4 ++--
 arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
 arch/x86/kernel/signal.c               | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/include/asm/sigcontext.h +2 -2
index f910cdcb71fd..5f0ef11719e1 100644
@@ -57,8 +57,8 @@ struct sigcontext {
 	unsigned long ip;
 	unsigned long flags;
 	unsigned short cs;
-	unsigned short gs;
-	unsigned short fs;
+	unsigned short __pad2;	/* Was called gs, but was always zero. */
+	unsigned short __pad1;	/* Was called gs, but was always zero. */
 	unsigned short ss;
 	unsigned long err;
 	unsigned long trapno;
arch/x86/include/uapi/asm/sigcontext.h +2 -2
index 076b11fd6fa1..df9908b1aa95 100644
@@ -177,8 +177,8 @@ struct sigcontext {
 	__u64 rip;
 	__u64 eflags;		/* RFLAGS */
 	__u16 cs;
-	__u16 gs;
-	__u16 fs;
+	__u16 __pad2;		/* Was called gs, but was always zero. */
+	__u16 __pad1;		/* Was called fs, but was always zero. */
 	__u16 ss;
 	__u64 err;
 	__u64 trapno;
arch/x86/kernel/signal.c +2 -2
index 40f34574fb36..01a53767823c 100644
@@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 #else /* !CONFIG_X86_32 */
 		put_user_ex(regs->flags, &sc->flags);
 		put_user_ex(regs->cs, &sc->cs);
-		put_user_ex(0, &sc->gs);
-		put_user_ex(0, &sc->fs);
+		put_user_ex(0, &sc->__pad2);
+		put_user_ex(0, &sc->__pad1);
 		put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
Signature · 1 line
2.3.0
author
#8 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[cc: Oleg, Borislav]

Quoted 63 lines · Andy Lutomirski

As far as I can tell, these fields have been set to zero on save and ignored on restore since Linux was imported into git. Rename them '__pad1' and '__pad2' to avoid confusion and to allow them to be recycled some day.

I'm intentionally avoiding calling either of them __pad0: the field formerly known as __pad0 is now ss.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/sigcontext.h      | 4 ++--
 arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
 arch/x86/kernel/signal.c               | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/include/asm/sigcontext.h +2 -2
index f910cdcb71fd..5f0ef11719e1 100644
@@ -57,8 +57,8 @@ struct sigcontext {
        unsigned long ip;
        unsigned long flags;
        unsigned short cs;
-       unsigned short gs;
-       unsigned short fs;
+       unsigned short __pad2;  /* Was called gs, but was always zero. */
+       unsigned short __pad1;  /* Was called gs, but was always zero. */
        unsigned short ss;
        unsigned long err;
        unsigned long trapno;
arch/x86/include/uapi/asm/sigcontext.h +2 -2
index 076b11fd6fa1..df9908b1aa95 100644
@@ -177,8 +177,8 @@ struct sigcontext {
        __u64 rip;
        __u64 eflags;           /* RFLAGS */
        __u16 cs;
-       __u16 gs;
-       __u16 fs;
+       __u16 __pad2;           /* Was called gs, but was always zero. */
+       __u16 __pad1;           /* Was called fs, but was always zero. */
        __u16 ss;
        __u64 err;
        __u64 trapno;
arch/x86/kernel/signal.c +2 -3
index 40f34574fb36..01a53767823c 100644
@@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 #else /* !CONFIG_X86_32 */
                put_user_ex(regs->flags, &sc->flags);
                put_user_ex(regs->cs, &sc->cs);
-               put_user_ex(0, &sc->gs);
-               put_user_ex(0, &sc->fs);
+               put_user_ex(0, &sc->__pad2);
+               put_user_ex(0, &sc->__pad1);
                put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
--

2.3.0

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#9 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

In reply to Andy Lutomirski
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Andy" == Andy Lutomirski <[email protected]> writes:

Andy> As far as I can tell, these fields have been set to zero on save and Andy> ignored on restore since Linux was imported into git. Rename them Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be Andy> recycled some day.

Andy> I'm intentionally avoiding calling either of them __pad0: the field Andy> formerly known as __pad0 is now ss.

Andy> Signed-off-by: Andy Lutomirski <[email protected]> Andy> ---

Andy>  arch/x86/include/asm/sigcontext.h      | 4 ++--
Andy>  arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
Andy>  arch/x86/kernel/signal.c               | 4 ++--
Andy>  3 files changed, 6 insertions(+), 6 deletions(-)

Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h Andy> index f910cdcb71fd..5f0ef11719e1 100644
Andy> --- a/arch/x86/include/asm/sigcontext.h
Andy> +++ b/arch/x86/include/asm/sigcontext.h
Andy> @@ -57,8 +57,8 @@ struct sigcontext {

Andy>  	unsigned long ip;
Andy>  	unsigned long flags;
Andy>  	unsigned short cs;

Andy> - unsigned short gs;
Andy> - unsigned short fs;
Andy> + unsigned short __pad2; /* Was called gs, but was always zero. */
Andy> + unsigned short __pad1; /* Was called gs, but was always zero. */

Shouldn't this comment read:

/* Was called fs, but was always zero. */

for the second one?

author
#10 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

Quoted 30 lines · John Stoffel
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Andy" == Andy Lutomirski <[email protected]> writes:

Andy> As far as I can tell, these fields have been set to zero on save and Andy> ignored on restore since Linux was imported into git. Rename them Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be Andy> recycled some day.

Andy> I'm intentionally avoiding calling either of them __pad0: the field Andy> formerly known as __pad0 is now ss.

Andy> Signed-off-by: Andy Lutomirski <[email protected]> Andy> ---

Andy>  arch/x86/include/asm/sigcontext.h      | 4 ++--
Andy>  arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
Andy>  arch/x86/kernel/signal.c               | 4 ++--
Andy>  3 files changed, 6 insertions(+), 6 deletions(-)

Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h Andy> index f910cdcb71fd..5f0ef11719e1 100644
Andy> --- a/arch/x86/include/asm/sigcontext.h
Andy> +++ b/arch/x86/include/asm/sigcontext.h
Andy> @@ -57,8 +57,8 @@ struct sigcontext {

Andy>   unsigned long ip;
Andy>   unsigned long flags;
Andy>   unsigned short cs;

Andy> - unsigned short gs;
Andy> - unsigned short fs;
Andy> + unsigned short __pad2; /* Was called gs, but was always zero. */ Andy> + unsigned short __pad1; /* Was called gs, but was always zero. */

Quoted 5 lines

Shouldn't this comment read:

/* Was called fs, but was always zero. */

for the second one?

Will fix for v2. Thanks.

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
maintainer Acked-by on v2 2/2
#11 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

In reply to Andy Lutomirski
Quoted 2 lines · Andy Lutomirski

As far as I can tell, these fields have been set to zero on save and ignored on restore since Linux was imported into git. Rename them

Actually from the very beginning:

commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75

Author: Andi Kleen <[email protected]>
Date:   Tue Feb 12 20:17:35 2002 -0800
    [PATCH] x86_64 merge: arch + asm

...

+static int
+setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
+ struct pt_regs *regs, unsigned long mask)
+{
+ int tmp, err = 0;
+

+       tmp = 0;
+       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->gs);
+       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->fs);
Quoted 7 lines

'__pad1' and '__pad2' to avoid confusion and to allow them to be recycled some day.

I'm intentionally avoiding calling either of them __pad0: the field formerly known as __pad0 is now ss.

Signed-off-by Andy Lutomirski <[email protected]>
Acked-by Borislav Petkov <[email protected]>
Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
author
#12 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

Quoted 17 lines · Borislav Petkov
Quoted 2 lines · Andy Lutomirski

As far as I can tell, these fields have been set to zero on save and ignored on restore since Linux was imported into git. Rename them

Actually from the very beginning:

commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75

Author: Andi Kleen <[email protected]>
Date:   Tue Feb 12 20:17:35 2002 -0800
    [PATCH] x86_64 merge: arch + asm

...

(quoted diff) +1 -0
+static int

Review

Quoted 10 lines
(quoted diff) +10 -0
+setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
+                struct pt_regs *regs, unsigned long mask)
+{
+       int tmp, err = 0;
+
+       tmp = 0;
+       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->gs);
+       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->fs);

Wait... It looks like it really was saving them. Of course, this is mostly useless unless the code also does something very clever with fsbase and gsbase.

After wrestling with the historic git a bit:

commit a104ba57b3b3697fd77778918b6a77ed16bb2ff8

Author: Andi Kleen <[email protected]>
Date:   Mon Mar 10 01:41:56 2003 -0800
    [PATCH] x86-64 updates for 2.5.64-bk3

...

static inline int
setup_sigcontext(struct sigcontext *sc, struct pt_regs *regs,
unsigned long mask, struct task_struct *me)
{
- int tmp, err = 0;
+ int err = 0;

-       tmp = 0;
-       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
-       err |= __put_user(tmp, (unsigned int *)&sc->gs);
-       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
-       err |= __put_user(tmp, (unsigned int *)&sc->fs);
+       err |= __put_user(0, &sc->gs);
+       err |= __put_user(0, &sc->fs);

Prior to that, we actually saved and restored the thing. In restore_context:

        {
                unsigned int seg;
                err |= __get_user(seg, &sc->gs);
                load_gs_index(seg);
                err |= __get_user(seg, &sc->fs);
                loadsegment(fs,seg);
        }

We did not, however, save and restore fsbase and gsbase.

Interestingly, in the same BK change, we also remove set_thread_area as a 64-bit syscall entirely (take that, ABI compatibility!), but arch_prctl existed before that change.

IOW, in 2.5.63 and earlier, we tried to save and restore TLS state across signals, but we did it wrong and would corrupt it for any program that used arch_prctl. At that time, programs could switch userspace threads using signals and their TLS pointers would switch. In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl users, and made signals effectively *not* switch TLS pointers.

It seems unlikely to me that many pre-2.5.64 programs still run due to the aforementioned removal of a syscall, but I think that we should at least document this in the header so that anyone who tries to recycle those padding bits is appropriately careful.

--Andy

maintainer
#13 lore raw

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

Quoted 1 line · Andy Lutomirski

Wait... It looks like it really was saving them. Of course, this is

Bah, of course. Blind me.

Quoted 5 lines

IOW, in 2.5.63 and earlier, we tried to save and restore TLS state across signals, but we did it wrong and would corrupt it for any program that used arch_prctl. At that time, programs could switch userspace threads using signals and their TLS pointers would switch. In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl

That is probably non-issue as 2.5 was the devel branch anyway in the old days. I'm not sure what the statement on breaking ABI was though then...

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
author
#14 lore raw
In reply to Andy Lutomirski

Sorry, forgot to CC Oleg and Borislav. Let me know if you want me to forward the rest of the thread to you.

Quoted 28 lines · Andy Lutomirski

Patch 1 is IMO a bug fix.

Patch 2 is a cleanup and avoids some confusion. It's also sort of an API break (no ABI change) because it removes a struct field. But that struct field has never done anything at all as far as I can tell.

AFAICT this series got bogged down in discussion about patch 2 last time. I'm only including patch 2 for completeness here -- patch 1 is IMO far more useful, and I'm fine with patch 1 being merged and tabling patch 2 again.

Changes from v1:
- Add the __USER_DS fix, caught by my sigreturn test.
- Re-tested locally.

Andy Lutomirski (2):
x86_64,signal: Fix SS handling for signals delivered to 64-bit
programs
x86_64,signal: Remove 'fs' and 'gs' from sigcontext

 arch/x86/include/asm/sigcontext.h      |  6 +++---
 arch/x86/include/uapi/asm/sigcontext.h |  6 +++---
 arch/x86/kernel/signal.c               | 24 +++++++++++++-----------
 3 files changed, 19 insertions(+), 17 deletions(-)

--
2.3.0

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC