Re: [PATCH 2/3] arm64/ptrace: introduce PTRACE_O_ARM64_RAW_REGS

[Andrei Vagin]

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