On Thu, Feb 04, 2021 at 03:36:15PM +0000, Will Deacon wrote:
> On Mon, Feb 01, 2021 at 11:40:11AM -0800, Andrei Vagin wrote:
> > We have some ABI weirdness in the way that we handle syscall
> > exit stops because we indicate whether or not the stop has been
> > signalled from syscall entry or syscall exit by clobbering a general
> > purpose register (ip/r12 for AArch32, x7 for AArch64) in the tracee
> > and restoring its old value after the stop.
> >
> > This behavior was inherited from ARM and it isn't common for other
> > architectures. Now, we have PTRACE_GET_SYSCALL_INFO that gives all
> > required information about system calls, so the hack with clobbering
> > registers isn't needed anymore.
> >
> > This change adds the new ptrace option PTRACE_O_ARM64_RAW_REGS. If it
> > is set, PTRACE_GETREGSET returns values of all registers without
> > clobbering r12 or x7 and PTRACE_SETREGSE sets all registers even if a
> > process has been stopped in syscall-enter or syscall-exit.
> >
> > Signed-off-by: Andrei Vagin <avagin@xxxxxxxxx>
> > ---
> > arch/arm64/include/uapi/asm/ptrace.h | 4 ++
> > arch/arm64/kernel/ptrace.c | 70 ++++++++++++++++------------
> > include/linux/ptrace.h | 1 +
> > include/uapi/linux/ptrace.h | 9 +++-
> > 4 files changed, 52 insertions(+), 32 deletions(-)
>
> Please split this up so that the arm64-specific changes are separate to
> the core changes.
ok
>
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index 83ee45fa634b..bcc8c362ddd9 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -7,6 +7,7 @@
> > /* has the defines to get at the registers. */
> >
> > #include <linux/types.h>
> > +#include <asm/ptrace.h>
> >
> > #define PTRACE_TRACEME 0
> > #define PTRACE_PEEKTEXT 1
> > @@ -137,8 +138,14 @@ struct ptrace_syscall_info {
> > #define PTRACE_O_EXITKILL (1 << 20)
> > #define PTRACE_O_SUSPEND_SECCOMP (1 << 21)
> >
> > +/* (1<<28) is reserved for arch specific options. */
> > +#ifndef _PTRACE_O_ARCH_OPTIONS
> > +#define _PTRACE_O_ARCH_OPTIONS 0
> > +#endif
>
> It seems a bit fragile to rely on a comment here to define the user ABI;
> why not define _PTRACE_O_ARCH_OPTIONS to the right value unconditionally?
We don't want to allow setting options that are not supported.
_PTRACE_O_ARCH_OPTIONS is added to PTRACE_O_MASK and then
ptrace_setoptions checks whether all specified options is supported or
not.
>
> Also, it seems as though we immediately burn this bit on arm64, so if we
> ever wanted another option we'd have to come back here and allocate another
> bit. Could we do better, e.g. by calling into an arch hook
> (arch_ptrace_setoptions()) and passing the 'addr' parameter?
I am not sure that I understand the idea. Do you suggest to have
PTRACE_O_ARCH_OPTION and pass arch-specific options in addr? In this
case, I think it could be more cleaner to introduce a new ptrace
command. If this is the idea, I think it worth doing this only if we
expect to have more than one,two,three options.
As for my solution, we need to come back to allocate a new bit
to be sure that we don't intersect with non-arch specific options.
And those who add a non-arch option should see that they don't use bits
of arch-specific options.
Let's decide what interface we want to use to solve the problem and then
if we will stop on a ptrace option I will figure out how to improve
this code.
>
> How do other architectures manage this sort of thing? I'm wondering whether
> a separate regset containing just "real x7" and orig_x0 would be preferable
> after all...
Yeah, it might be a good idea. We will need to do one extra ptrace
system call, but in comparison with ptrace context-switches, this is
nothing.
Dave, Keno, what do you think about this?
>
> Will
