x86/signal: Remove pax argument from restore_sigcontext

The pax argument is unnecesary. Instead, store the RAX value directly in regs.

Signed-off-by Brian Gerst <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 arch/x86/ia32/ia32_signal.c        | 17 ++++++-----------
 arch/x86/include/asm/sighandling.h |  4 +---
 arch/x86/kernel/signal.c           | 22 ++++++++--------------
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/x86/ia32/ia32_signal.c +2 -3

index 1f5e2b0..c81d35e6 100644
@@ -161,8 +161,7 @@ int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
-				   struct sigcontext_ia32 __user *sc,
-				   unsigned int *pax)
+				   struct sigcontext_ia32 __user *sc)
 {
 	unsigned int tmpflags, err = 0;
 	void __user *buf;

@@ -184,7 +183,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		RELOAD_SEG(es);
 
 		COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
-		COPY(dx); COPY(cx); COPY(ip);
+		COPY(dx); COPY(cx); COPY(ip); COPY(ax);
 		/* Don't touch extended registers */
 
 		COPY_SEG_CPL3(cs);

Quoted 43 lines · Brian Gerst

The pax argument is unnecesary. Instead, store the RAX value directly in regs.

Signed-off-by Brian Gerst <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 arch/x86/ia32/ia32_signal.c        | 17 ++++++-----------
 arch/x86/include/asm/sighandling.h |  4 +---
 arch/x86/kernel/signal.c           | 22 ++++++++--------------
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/x86/ia32/ia32_signal.c +2 -3

index 1f5e2b0..c81d35e6 100644
@@ -161,8 +161,7 @@ int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
-                                struct sigcontext_ia32 __user *sc,
-                                unsigned int *pax)
+                                struct sigcontext_ia32 __user *sc)
 {
      unsigned int tmpflags, err = 0;
      void __user *buf;

@@ -184,7 +183,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
              RELOAD_SEG(es);
 
              COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
-             COPY(dx); COPY(cx); COPY(ip);
+             COPY(dx); COPY(cx); COPY(ip); COPY(ax);
              /* Don't touch extended registers */
 
              COPY_SEG_CPL3(cs);

@@ -197,8 +196,6 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 

arch/x86/ia32/ia32_signal.c static int ia32_restore_sigcontext(struct pt_regs *regs, · line 199

Quoted 5 lines

arch/x86/ia32/ia32_signal.c +0 -2

              get_user_ex(tmp, &sc->fpstate);
              buf = compat_ptr(tmp);
-
-             get_user_ex(*pax, &sc->ax);
      } get_user_catch(err);

Note that arch/x86/kernel/signal.c appears to have a similar pattern - and there it could be removed as well?

I'm wondering what the original reason for adding the extra handling of regs->ax was. Maybe something changed regs->ax - but I cannot find such code path anymore.

It would be nice to try to do a bit of Git archeology to figure out the origins of this complication - maybe it's something subtle - or it's something that has changed meanwhile.

Quoted 46 lines · Ingo Molnar

Quoted 43 lines · Brian Gerst

The pax argument is unnecesary. Instead, store the RAX value directly in regs.

Signed-off-by Brian Gerst <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 arch/x86/ia32/ia32_signal.c        | 17 ++++++-----------
 arch/x86/include/asm/sighandling.h |  4 +---
 arch/x86/kernel/signal.c           | 22 ++++++++--------------
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/x86/ia32/ia32_signal.c +2 -3

index 1f5e2b0..c81d35e6 100644
@@ -161,8 +161,7 @@ int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
-                                struct sigcontext_ia32 __user *sc,
-                                unsigned int *pax)
+                                struct sigcontext_ia32 __user *sc)
 {
      unsigned int tmpflags, err = 0;
      void __user *buf;

@@ -184,7 +183,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
              RELOAD_SEG(es);
 
              COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
-             COPY(dx); COPY(cx); COPY(ip);
+             COPY(dx); COPY(cx); COPY(ip); COPY(ax);
              /* Don't touch extended registers */
 
              COPY_SEG_CPL3(cs);

@@ -197,8 +196,6 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 

Quoted 8 lines

arch/x86/ia32/ia32_signal.c static int ia32_restore_sigcontext(struct pt_regs *regs, · line 199

Quoted 5 lines

arch/x86/ia32/ia32_signal.c +0 -2

              get_user_ex(tmp, &sc->fpstate);
              buf = compat_ptr(tmp);
-
-             get_user_ex(*pax, &sc->ax);
      } get_user_catch(err);

Note that arch/x86/kernel/signal.c appears to have a similar pattern - and there it could be removed as well?

I'm guessing you didn't read the whole patch, because I did change it.

Quoted 42 lines · Brian Gerst

The pax argument is unnecesary. Instead, store the RAX value directly in regs.

Signed-off-by Brian Gerst <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 arch/x86/ia32/ia32_signal.c        | 17 ++++++-----------
 arch/x86/include/asm/sighandling.h |  4 +---
 arch/x86/kernel/signal.c           | 22 ++++++++--------------
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/x86/ia32/ia32_signal.c +2 -3

index 1f5e2b0..c81d35e6 100644
@@ -161,8 +161,7 @@ int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
-                                struct sigcontext_ia32 __user *sc,
-                                unsigned int *pax)
+                                struct sigcontext_ia32 __user *sc)
 {
      unsigned int tmpflags, err = 0;
      void __user *buf;

@@ -184,7 +183,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
              RELOAD_SEG(es);
 
              COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
-             COPY(dx); COPY(cx); COPY(ip);
+             COPY(dx); COPY(cx); COPY(ip); COPY(ax);
              /* Don't touch extended registers */
 
              COPY_SEG_CPL3(cs);

arch/x86/ia32/ia32_signal.c static int ia32_restore_sigcontext(struct pt_regs *regs, · line 199

Quoted 6 lines

arch/x86/ia32/ia32_signal.c +0 -2

@@ -197,8 +196,6 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 
              get_user_ex(tmp, &sc->fpstate);
              buf = compat_ptr(tmp);
-
-             get_user_ex(*pax, &sc->ax);
      } get_user_catch(err);

Note that arch/x86/kernel/signal.c appears to have a similar pattern - and there it could be removed as well?

I'm wondering what the original reason for adding the extra handling of regs->ax was. Maybe something changed regs->ax - but I cannot find such code path anymore.

It would be nice to try to do a bit of Git archeology to figure out the origins of this complication - maybe it's something subtle - or it's something that has changed meanwhile.

Quoted 4 lines

I'm wondering what the original reason for adding the extra handling of regs->ax was. Maybe something changed regs->ax - but I cannot find such code path anymore.

Quoted 3 lines

It would be nice to try to do a bit of Git archeology to figure out the origins of this complication - maybe it's something subtle
- or it's something that has changed meanwhile.

It goes all the way back to 2.1.106pre1, when restore_sigcontext() was changed to return an error code instead of EAX directly.

https://git.kernel.org/cgit/linux/kernel/git/history/history.git/diff/arch/i386/kernel/signal.c?id=9a8f8b7ca3f319bd668298d447bdf32730e51174

Quoted 11 lines · Brian Gerst

Quoted 7 lines

I'm wondering what the original reason for adding the extra handling of regs->ax was. Maybe something changed regs->ax - but I cannot find such code path anymore.

It would be nice to try to do a bit of Git archeology to figure out the origins of this complication - maybe it's something subtle
- or it's something that has changed meanwhile.

It goes all the way back to 2.1.106pre1, when restore_sigcontext() was changed to return an error code instead of EAX directly.

Quoted 1 line

https://git.kernel.org/cgit/linux/kernel/git/history/history.git/diff/arch/i386/kernel/signal.c?id=9a8f8b7ca3f319bd668298d447bdf32730e51174

Indeed: restore_sigcontext() used to return eax as a return value, without copying it into regs->ax.

Then in 2007

sigaltstack syscall support was added, where the return value of restore_sigcontext() was changed to carry the memory-copying failure code. But instead of putting 'ax' into regs->ax, it was carried in via a pointer and then returned, where the generic syscall return code copied it to regs->ax.

So there was never any deeper reason for this suboptimal pattern, it was simply never noticed after being introduced.

(Btw., the regs->ax we return will be copied back to regs->ax after the syscall straight away once again - but I guess this cannot be helped.)

Quoted 14 lines · Ingo Molnar

Quoted 11 lines · Brian Gerst

Quoted 7 lines

I'm wondering what the original reason for adding the extra handling of regs->ax was. Maybe something changed regs->ax - but I cannot find such code path anymore.

It would be nice to try to do a bit of Git archeology to figure out the origins of this complication - maybe it's something subtle
- or it's something that has changed meanwhile.

It goes all the way back to 2.1.106pre1, when restore_sigcontext() was changed to return an error code instead of EAX directly.

Quoted 6 lines

Quoted 1 line

https://git.kernel.org/cgit/linux/kernel/git/history/history.git/diff/arch/i386/kernel/signal.c?id=9a8f8b7ca3f319bd668298d447bdf32730e51174

Indeed: restore_sigcontext() used to return eax as a return value, without copying it into regs->ax.

Then in 2007

Version 2.1.106 was released on Jun 13, 1998.

Quoted 6 lines

sigaltstack syscall support was added, where the return value of restore_sigcontext() was changed to carry the memory-copying failure code. But instead of putting 'ax' into regs->ax, it was carried in via a pointer and then returned, where the generic syscall return code copied it to regs->ax.

Quoted 6 lines

So there was never any deeper reason for this suboptimal pattern, it was simply never noticed after being introduced.

(Btw., the regs->ax we return will be copied back to regs->ax after the syscall straight away once again - but I guess this cannot be helped.)

The 64-bit stub could skip saving it back to regs.