x86: fix get_nr_restart_syscall()

Somehow I forgot about this problem. Let me resend the last version based on discussion with Linus. IIRC he was agree with this series.

Yeah, looks sane to me.

The comment in get_nr_restart_syscall() says:

* The problem is that we can get here when ptrace pokes
* syscall-like values into regs even if we're not in a syscall
* at all.

Yes. but if we are not in syscall then the

        status & (TS_COMPAT|TS_I386_REGS_POKED)

check below can't really help:

- TS_COMPAT can't be set

- TS_I386_REGS_POKED is only set if regs->orig_ax was changed by 32bit debugger; and even in this case get_nr_restart_syscall() is only correct if the tracee is 32bit too.

Suppose that 64bit debugger plays with 32bit tracee and

* Tracee calls sleep(2) // TS_COMPAT is set
* User interrupts the tracee by CTRL-C after 1 sec and does "(gdb) call func()"
* gdb saves the regs by PTRACE_GETREGS

* does PTRACE_SETREGS to set %rip='func' and %orig_rax=-1
* PTRACE_CONT // TS_COMPAT is cleared
* func() hits int3.
* Debugger catches SIGTRAP.
* Restore original regs by PTRACE_SETREGS.
* PTRACE_CONT

get_nr_restart_syscall() wrongly returns __NR_restart_syscall==219, the tracee calls ia32_sys_call_table[219] == sys_madvise.

This patch adds the sticky TS_COMPAT_RESTART flag which survives after return to user mode, hopefully it allows us to kill TS_I386_REGS_POKED but this needs another patch.

Test-case:

  $ cvs -d :pserver:anoncvs:[email protected]:/cvs/systemtap co ptrace-tests
  $ gcc -o erestartsys-trap-debuggee ptrace-tests/tests/erestartsys-trap-debuggee.c --m32
  $ gcc -o erestartsys-trap-debugger ptrace-tests/tests/erestartsys-trap-debugger.c -lutil
  $ ./erestartsys-trap-debugger
  Unexpected: retval 1, errno 22
  erestartsys-trap-debugger: ptrace-tests/tests/erestartsys-trap-debugger.c:421

Reported-by Jan Kratochvil <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/thread_info.h | 14 +++++++++++++-
 arch/x86/kernel/signal.c           | 24 +-----------------------
 2 files changed, 14 insertions(+), 24 deletions(-)

arch/x86/include/asm/thread_info.h +13 -1

index c2dc29e215ea..30d1d187019f 100644
@@ -214,10 +214,22 @@ static inline int arch_within_stack_frames(const void * const stack,
  */
 #define TS_COMPAT              0x0002  /* 32bit syscall active (64BIT)*/
 
+#ifndef __ASSEMBLY__
 #ifdef CONFIG_COMPAT
 #define TS_I386_REGS_POKED     0x0004  /* regs poked by 32-bit ptracer */
+#define TS_COMPAT_RESTART      0x0008
+
+#define arch_set_restart_data  arch_set_restart_data
+
+static inline void arch_set_restart_data(struct restart_block *restart)
+{
+       struct thread_info *ti = current_thread_info();
+       if (ti->status & TS_COMPAT)
+               ti->status |= TS_COMPAT_RESTART;
+       else
+               ti->status &= ~TS_COMPAT_RESTART;
+}
 #endif
-#ifndef __ASSEMBLY__
 
 #ifdef CONFIG_X86_32
 #define in_ia32_syscall() true

arch/x86/kernel/signal.c +1 -24

index ea794a083c44..6c26d2c3a2e4 100644
@@ -766,30 +766,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 
 static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 {
-       /*
-        * This function is fundamentally broken as currently
-        * implemented.
-        *
-        * The idea is that we want to trigger a call to the
-        * restart_block() syscall and that we want in_ia32_syscall(),
-        * in_x32_syscall(), etc. to match whatever they were in the
-        * syscall being restarted.  We assume that the syscall
-        * instruction at (regs->ip - 2) matches whatever syscall
-        * instruction we used to enter in the first place.
-        *
-        * The problem is that we can get here when ptrace pokes
-        * syscall-like values into regs even if we're not in a syscall
-        * at all.
-        *
-        * For now, we maintain historical behavior and guess based on
-        * stored state.  We could do better by saving the actual
-        * syscall arch in restart_block or (with caveats on x32) by
-        * checking if regs->ip points to 'int $0x80'.  The current
-        * behavior is incorrect if a tracer has a different bitness
-        * than the tracee.
-        */
 #ifdef CONFIG_IA32_EMULATION
-       if (current_thread_info()->status & (TS_COMPAT|TS_I386_REGS_POKED))
+       if (current_thread_info()->status & TS_COMPAT_RESTART)
                return __NR_ia32_restart_syscall;
 #endif
 #ifdef CONFIG_X86_X32_ABI
--

2.25.1.362.g51ebf55

Quoted 13 lines · Oleg Nesterov

The comment in get_nr_restart_syscall() says:

* The problem is that we can get here when ptrace pokes
* syscall-like values into regs even if we're not in a syscall
* at all.

Yes. but if we are not in syscall then the

        status & (TS_COMPAT|TS_I386_REGS_POKED)

check below can't really help:

Quoted 7 lines

- TS_COMPAT can't be set

- TS_I386_REGS_POKED is only set if regs->orig_ax was changed by 32bit debugger; and even in this case get_nr_restart_syscall() is only correct if the tracee is 32bit too.

Suppose that 64bit debugger plays with 32bit tracee and

At the risk of asking an obnoxious question here:

Quoted 1 line

Quoted 4 lines

* Tracee calls sleep(2) // TS_COMPAT is set
* User interrupts the tracee by CTRL-C after 1 sec and does "(gdb) call func()"
* gdb saves the regs by PTRACE_GETREGS

It seems to me that a better solution may be for gdb to see the post-restart-setup state. In other words, shouldn't the GETREGS return with the ax pointing to the restart syscall already?

Quoted 2 lines · Linus Torvalds

Yeah, looks sane to me.

Oh, and in a perfect world we'd have a test for this condition too, no?

Quoted 26 lines · Andy Lutomirski

Quoted 13 lines · Oleg Nesterov

The comment in get_nr_restart_syscall() says:

* The problem is that we can get here when ptrace pokes
* syscall-like values into regs even if we're not in a syscall
* at all.

Yes. but if we are not in syscall then the

        status & (TS_COMPAT|TS_I386_REGS_POKED)

check below can't really help:

Quoted 7 lines

- TS_COMPAT can't be set

- TS_I386_REGS_POKED is only set if regs->orig_ax was changed by 32bit debugger; and even in this case get_nr_restart_syscall() is only correct if the tracee is 32bit too.

Suppose that 64bit debugger plays with 32bit tracee and

At the risk of asking an obnoxious question here:

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

* Tracee calls sleep(2) // TS_COMPAT is set
* User interrupts the tracee by CTRL-C after 1 sec and does "(gdb) call func()"
* gdb saves the regs by PTRACE_GETREGS

It seems to me that a better solution may be for gdb to see the post-restart-setup state. In other words, shouldn't the GETREGS return with the ax pointing to the restart syscall already?

and ip = regs-ip - 2? And hide ERESTART_BLOCK from debugger? Perhaps I misunderstood, but this doesn't look like a better solution to me. Not to mention this would be the serious user-visible change... And even the necessary changes in getreg() do not look good to me.

Plus I do not understand how this could work. OK, suppose that the tracee reports a signal with ax = ERESTART_BLOCK.

Debugger simply does GETREGS + SETREGS + PTRACE_CONT(signr). In this case handle_signal() should set ax = -EINTR, but syscall_get_error() will report __NR_ia32_restart_syscall?

Probably I greatly misunderstood you...

There is the "erestartsys-trap-debugger" test in ptrace-tests suite. Do you mean you want another test in tools/testing/selftests/ptrace ?

Quoted 3 lines · Oleg Nesterov

There is the "erestartsys-trap-debugger" test in ptrace-tests suite. Do you mean you want another test in tools/testing/selftests/ptrace ?

No, I guess it's fine if it's caught by the ptrace test suite - we'll hopefully get fairly timely "guys, you broke it" reports.

Is that ptrace erestartsys-trap-debugger.c test new, or has it just always failed? Or is the problem that it is so expected to fail that we wouldn't get reports of it anyway (this clearly fell off your radar for a long time)?

Somehow I forgot about this problem. Let me resend the last version based on discussion with Linus. IIRC he was agree with this series.

And let me remind why 3/4 temporary adds the "transient" TS_COMPAT_RESTART flag killed by the next patch: to simplify the backporting. 1-3 can fix the problem without breaking the kABI.

Oleg.

---
 arch/x86/include/asm/processor.h   |  9 ---------
 arch/x86/include/asm/thread_info.h | 15 ++++++++++++++-
 arch/x86/kernel/signal.c           | 24 +-----------------------
 fs/select.c                        | 10 ++++------
 include/linux/restart_block.h      |  1 +
 include/linux/thread_info.h        | 13 +++++++++++++
 kernel/futex.c                     |  3 +--
 kernel/time/alarmtimer.c           |  2 +-
 kernel/time/hrtimer.c              |  2 +-
 kernel/time/posix-cpu-timers.c     |  2 +-
 10 files changed, 37 insertions(+), 44 deletions(-)

It seems that nobody objects,

Andrew, Andy, Thomas, how do you think this series should be routed?

Quoted 21 lines

Somehow I forgot about this problem. Let me resend the last version based on discussion with Linus. IIRC he was agree with this series.

And let me remind why 3/4 temporary adds the "transient" TS_COMPAT_RESTART flag killed by the next patch: to simplify the backporting. 1-3 can fix the problem without breaking the kABI.

Oleg.

---
 arch/x86/include/asm/processor.h   |  9 ---------
 arch/x86/include/asm/thread_info.h | 15 ++++++++++++++-
 arch/x86/kernel/signal.c           | 24 +-----------------------
 fs/select.c                        | 10 ++++------
 include/linux/restart_block.h      |  1 +
 include/linux/thread_info.h        | 13 +++++++++++++
 kernel/futex.c                     |  3 +--
 kernel/time/alarmtimer.c           |  2 +-
 kernel/time/hrtimer.c              |  2 +-
 kernel/time/posix-cpu-timers.c     |  2 +-
 10 files changed, 37 insertions(+), 44 deletions(-)

Quoted 4 lines · Oleg Nesterov

It seems that nobody objects,

Andrew, Andy, Thomas, how do you think this series should be routed?

ping...

What can I do to finally get this merged?

Should I resend once again? Whom?