x86/signal: fix restart_syscall number for x32 tasks

PATCH Superseded Andy Lutomirski 4 participants 9 messages Dec 7, 2015 – Jan 19, 2016 x86

Share & tools

Andy Lutomirski

author

2015-12-07 23:22 UTC #0 lore raw

[not real reply because I'm using a bad internet connection right now and I'm not set up with my usual Gmane reply hack right now]

The new code is (whitespace-damaged):

static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) {
#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
return __NR_restart_syscall;
#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
}

This is IMO awful. This use of TIF_IA32 is wrong, and this is otherwise gross. Can we do it for real:

if (is_ia32_task())
return __NR_ia32_restart_syscall;
else
return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
/* preserve x32 bit */

I'd send the patch myself, but you apparently have a good test case for this, and I don't.

(this isn't a regression, and I'm not suggesting any change for 4.4 or for stable. But for 4.5, can we do it right, please?)

And yes, I'll send a patch to rename is_ia32_task, but that's orthogonal.

--Andy

P.S. I'm still hoping to kill TIF_IA32 entirely some time soon.

Dmitry V. Levin

2015-12-13 03:44 UTC #1 lore raw

Quoted 18 lines · Andy Lutomirski

[not real reply because I'm using a bad internet connection right now and I'm not set up with my usual Gmane reply hack right now]
The new code is (whitespace-damaged):
static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) {
#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
return __NR_restart_syscall;
#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
}
This is IMO awful. This use of TIF_IA32 is wrong, and this is otherwise gross. Can we do it for real:

Quoted 8 lines

if (is_ia32_task())
return __NR_ia32_restart_syscall;
else
return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
/* preserve x32 bit */
I'd send the patch myself, but you apparently have a good test case for this, and I don't.

Unfortunately, this won't compile on CONFIG_X86_32 because __NR_ia32_restart_syscall is defined for CONFIG_X86_64 only.

Something like this should work:

static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) {

#ifdef CONFIG_X86_64
	if (is_ia32_task())
		return __NR_ia32_restart_syscall;
# ifdef CONFIG_X86_X32_ABI
	if (regs->orig_ax & __X32_SYSCALL_BIT)
		return __NR_restart_syscall | __X32_SYSCALL_BIT;
# endif
#endif
	return __NR_restart_syscall;
}

I don't see any way to avoid ifdefs here, sorry.

Signature · 1 line

ldv

Andy Lutomirski

author

2015-12-17 20:27 UTC #2 lore raw

Quoted 35 lines · Dmitry V. Levin

Quoted 18 lines · Andy Lutomirski
[not real reply because I'm using a bad internet connection right now and I'm not set up with my usual Gmane reply hack right now]
The new code is (whitespace-damaged):
static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) {
#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
return __NR_restart_syscall;
#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
}
This is IMO awful. This use of TIF_IA32 is wrong, and this is otherwise gross. Can we do it for real:
Quoted 8 lines
if (is_ia32_task())
return __NR_ia32_restart_syscall;
else
return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
/* preserve x32 bit */
I'd send the patch myself, but you apparently have a good test case for this, and I don't.
Unfortunately, this won't compile on CONFIG_X86_32 because __NR_ia32_restart_syscall is defined for CONFIG_X86_64 only.
Something like this should work:
static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) {

Quoted 10 lines

#ifdef CONFIG_X86_64
        if (is_ia32_task())
                return __NR_ia32_restart_syscall;
# ifdef CONFIG_X86_X32_ABI
        if (regs->orig_ax & __X32_SYSCALL_BIT)
                return __NR_restart_syscall | __X32_SYSCALL_BIT;
# endif
#endif
        return __NR_restart_syscall;
}

Looks good to me. Want to send a patch?

--Andy

Dmitry V. Levin

2015-12-18 23:37 UTC #3 lore raw

PATCH 1/1 x86/signal: Cleanup get_nr_restart_syscall

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.
Check for __X32_SYSCALL_BIT only if CONFIG_X86_X32_ABI is defined.

Signed-off-by Dmitry V. Levin <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/signal.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

arch/x86/kernel/signal.c +8 -5

index cb6282c..ff7dedc 100644
@@ -692,12 +692,15 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
 static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 {
-#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
+#ifdef CONFIG_X86_64
+	if (is_ia32_task())
+		return __NR_ia32_restart_syscall;
+# ifdef CONFIG_X86_X32_ABI
+	if (regs->orig_ax & __X32_SYSCALL_BIT)
+		return __NR_restart_syscall | __X32_SYSCALL_BIT;
+# endif
+#endif
 	return __NR_restart_syscall;
-#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
-	return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
-		__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
-#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
 }
 
 /*

Signature · 1 line

ldv

H. Peter Anvin

reviewer

2015-12-19 06:35 UTC #4 lore raw

Re: [PATCH] x86/signal: Cleanup get_nr_restart_syscall

Quoted 27 lines · Dmitry V. Levin

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.
Check for __X32_SYSCALL_BIT only if CONFIG_X86_X32_ABI is defined.
Signed-off-by Dmitry V. Levin <[email protected]>
Cc Elvira Khabirova <[email protected]>
Cc Andy Lutomirski <[email protected]>
---
  arch/x86/kernel/signal.c | 13 ++++++++-----
  1 file changed, 8 insertions(+), 5 deletions(-)
arch/x86/kernel/signal.c +6 -1
index cb6282c..ff7dedc 100644
@@ -692,12 +692,15 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
  static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
  {
-#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
+#ifdef CONFIG_X86_64
+	if (is_ia32_task())
+		return __NR_ia32_restart_syscall;
+# ifdef CONFIG_X86_X32_ABI
+	if (regs->orig_ax & __X32_SYSCALL_BIT)
+		return __NR_restart_syscall | __X32_SYSCALL_BIT;

arch/x86/kernel/signal.c handle_signal(struct ksignal *ksig, struct pt_regs *regs) · line 706

Quoted 10 lines

arch/x86/kernel/signal.c +2 -4

+# endif
+#endif
  	return __NR_restart_syscall;
-#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
-	return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
-		__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
-#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
  }
 
  /*

I bet you actually made the code slower.

-hpa

Dmitry V. Levin

2015-12-19 14:43 UTC #5 lore raw

PATCH v2 1/1 x86/signal: Cleanup get_nr_restart_syscall

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.
Check for __X32_SYSCALL_BIT iff CONFIG_X86_X32_ABI is defined.

Signed-off-by Dmitry V. Levin <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Andy Lutomirski <[email protected]>

---
v2: reintroduced __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT)

 arch/x86/kernel/signal.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

arch/x86/kernel/signal.c +8 -5

index cb6282c..c07ff5d 100644
@@ -692,12 +692,15 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
 static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 {
-#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
+#ifdef CONFIG_X86_64
+	if (is_ia32_task())
+		return __NR_ia32_restart_syscall;
+#endif
+#ifdef CONFIG_X86_X32_ABI
+	return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
+#else
 	return __NR_restart_syscall;
-#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
-	return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
-		__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
-#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
+#endif
 }
 
 /*

Signature · 1 line

ldv

Andy Lutomirski

author

2015-12-19 20:48 UTC #6 lore raw

Re: [PATCH v2] x86/signal: Cleanup get_nr_restart_syscall

Quoted 3 lines · Dmitry V. Levin

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.
Check for __X32_SYSCALL_BIT iff CONFIG_X86_X32_ABI is defined.

LGTM.

--Andy

Dmitry V. Levin

2016-01-11 14:55 UTC #7 lore raw

PATCH RESEND v2 1/1 x86/signal: Cleanup get_nr_restart_syscall

Date: Thu Dec 17 23:56:52 2015 +0000

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.
Check for __X32_SYSCALL_BIT iff CONFIG_X86_X32_ABI is defined.

Signed-off-by Dmitry V. Levin <[email protected]>

Cc Elvira Khabirova <[email protected]>

Acked-by Andy Lutomirski <[email protected]>

---
v2: reintroduced __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT)

 arch/x86/kernel/signal.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

arch/x86/kernel/signal.c +8 -5

index cb6282c..c07ff5d 100644
@@ -692,12 +692,15 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
 static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 {
-#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
+#ifdef CONFIG_X86_64
+	if (is_ia32_task())
+		return __NR_ia32_restart_syscall;
+#endif
+#ifdef CONFIG_X86_X32_ABI
+	return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
+#else
 	return __NR_restart_syscall;
-#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
-	return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
-		__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
-#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
+#endif
 }
 
 /*

Signature · 1 line

ldv

tip-bot for Dmitry V. Levin

merge bot

2016-01-19 13:40 UTC #8 lore raw

[tip:x86/asm] x86/signal: Cleanup get_nr_restart_syscall()

x86/signal: Cleanup get_nr_restart_syscall()

Commit-ID:  95d97adb2bb85d964bae4538e0574e742e522dda
Gitweb:     http://git.kernel.org/tip/95d97adb2bb85d964bae4538e0574e742e522dda
Author:     Dmitry V. Levin <[email protected]>
AuthorDate: Thu, 17 Dec 2015 23:56:52 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 19 Jan 2016 12:55:47 +0100

x86/signal: Cleanup get_nr_restart_syscall()

Check for TS_COMPAT instead of TIF_IA32 to distinguish ia32 tasks from 64-bit tasks.

Check for __X32_SYSCALL_BIT iff CONFIG_X86_X32_ABI is defined.

Signed-off-by Dmitry V. Levin <[email protected]>

Acked-by Andy Lutomirski <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/signal.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

arch/x86/kernel/signal.c +8 -5

index cb6282c..c07ff5d 100644
@@ -692,12 +692,15 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
 static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 {
-#if defined(CONFIG_X86_32) || !defined(CONFIG_X86_64)
+#ifdef CONFIG_X86_64
+	if (is_ia32_task())
+		return __NR_ia32_restart_syscall;
+#endif
+#ifdef CONFIG_X86_X32_ABI
+	return __NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
+#else
 	return __NR_restart_syscall;
-#else /* !CONFIG_X86_32 && CONFIG_X86_64 */
-	return test_thread_flag(TIF_IA32) ? __NR_ia32_restart_syscall :
-		__NR_restart_syscall | (regs->orig_ax & __X32_SYSCALL_BIT);
-#endif /* CONFIG_X86_32 || !CONFIG_X86_64 */
+#endif
 }
 
 /*