Re: [PATCH 08/17] ptrace/m68k: Stop open coding ptrace_report_syscall

[Finn Thain]

On Tue, 11 Jan 2022, Michael Schmitz wrote:

> Am 11.01.2022 um 06:54 schrieb Geert Uytterhoeven:
> > Hi Al,
> >
> > CC Michael/m68k,
> >
> > On Mon, Jan 10, 2022 at 5:20 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >> On Mon, Jan 10, 2022 at 04:26:57PM +0100, Geert Uytterhoeven wrote:
> >>> On Mon, Jan 3, 2022 at 10:33 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> >>> wrote:
> >>>> The generic function ptrace_report_syscall does a little more
> >>>> than syscall_trace on m68k.  The function ptrace_report_syscall
> >>>> stops early if PT_TRACED is not set, it sets ptrace_message,
> >>>> and returns the result of fatal_signal_pending.
> >>>>
> >>>> Setting ptrace_message to a passed in value of 0 is effectively not
> >>>> setting ptrace_message, making that additional work a noop.
> >>>>
> >>>> Returning the result of fatal_signal_pending and letting the caller
> >>>> ignore the result becomes a noop in this change.
> >>>>
> >>>> When a process is ptraced, the flag PT_PTRACED is always set in
> >>>> current->ptrace.  Testing for PT_PTRACED in ptrace_report_syscall is
> >>>> just an optimization to fail early if the process is not ptraced.
> >>>> Later on in ptrace_notify, ptrace_stop will test current->ptrace under
> >>>> tasklist_lock and skip performing any work if the task is not ptraced.
> >>>>
> >>>> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> >>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> >>>
> >>> As this depends on the removal of a parameter from
> >>> ptrace_report_syscall() earlier in this series:
> >>> Acked-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> >>
> >> FWIW, I would suggest taking it a bit further: make syscall_trace_enter()
> >> and syscall_trace_leave() in m68k ptrace.c unconditional, replace the
> >> calls of syscall_trace() in entry.S with syscall_trace_enter() and
> >> syscall_trace_leave() resp. and remove syscall_trace().
> >>
> >> Geert, do you see any problems with that?  The only difference is that
> >> current->ptrace_message would be set to 1 for ptrace stop on entry and
> >> 2 - on leave.  Currently m68k just has it 0 all along.
> >>
> >> It is user-visible (the whole point is to let the tracer see which
> >> stop it is - entry or exit one), so somebody using PTRACE_GETEVENTMSG
> >> on syscall stops would start seeing 1 or 2 instead of "0 all along".
> >> That's how it works on all other architectures (including m68k-nommu),
> >> and I doubt that anything in userland will get broken.
> >>
> >> Behaviour of PTRACE_GETEVENTMSG for other stops (fork, etc.) remains
> >> as-is, of course.
> >
> > In fact Michael did so in "[PATCH v7 1/2] m68k/kernel - wire up
> > syscall_trace_enter/leave for m68k"[1], but that's still stuck...
> >
> > [1]
> > https://lore.kernel.org/r/1624924520-17567-2-git-send-email-schmitzmic@xxxxxxxxx/
> 
> That patch (for reasons I never found out) did interact badly with 
> Christoph Hellwig's 'remove set_fs' patches (and Al's signal fixes which 
> Christoph's patches are based upon). Caused format errors under memory 
> stress tests quite reliably, on my 030 hardware.
> 

Those patches have since been merged, BTW.

> Probably needs a fresh look - the signal return path got changed by Al's 
> patches IIRC, and I might have relied on offsets to data on the stack 
> that are no longer correct with these patches. Or there's a race between 
> the syscall trap and signal handling when returning from interrupt 
> context ...
> 
> Still school hols over here so I won't have much peace and quiet until 
> February.
> 

So the patch works okay with Aranym 68040 but not Motorola 68030? Since 
there is at least one known issue affecting both Motorola 68030 and Hatari 
68030, perhaps this patch is not the problem. In anycase, Al's suggestion 
to split the patch into two may help in that testing two smaller patches 
might narrow down the root cause.