Re: [PATCH v4 3/4] x86: introduce TS_COMPAT_RESTART to fix

[Andy Lutomirski]

On Tue, Feb 2, 2021 at 7:03 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 02/01, Andy Lutomirski wrote:
> >
> > On Mon, Feb 1, 2021 at 9:47 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > 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
> >
> > At the risk of asking an obnoxious question here:
> >
> > >
> > >         * 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...

My idea may well be nuts, but I was indeed imagining that we hide
ERESTART_BLOCK from the debugger.  We would do the ip -= 2 and nr =
__NR_..._restart_syscall before any ptrace events happen at all.
Admittedly, this may cause strace to fail, so this is probably a bad
idea.

Oh well, your patch is probably a decent solution.