Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 28 Jan 2021 23:56:59 -0800
From:   Andrei Vagin <avagin@gmail.com>
To:     Dave Martin <Dave.Martin@arm.com>
Cc:     Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Oleg Nesterov <oleg@redhat.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] arm64/ptrace: introduce NT_ARM_PRSTATUS to get a
 full set of registers
Message-ID: <20210129075659.GA155266@gmail.com>
References: <20210119220637.494476-1-avagin@gmail.com>
 <20210119220637.494476-3-avagin@gmail.com>
 <20210127145304.GC13952@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
In-Reply-To: <20210127145304.GC13952@arm.com>
Precedence: bulk

On Wed, Jan 27, 2021 at 02:53:07PM +0000, Dave Martin wrote:
> On Tue, Jan 19, 2021 at 02:06:36PM -0800, Andrei Vagin wrote:
> > This is an alternative to NT_PRSTATUS that clobbers ip/r12 on AArch32,
> > x7 on AArch64 when a tracee is stopped in syscall entry or syscall exit
> > traps.
> > 
> > Signed-off-by: Andrei Vagin <avagin@gmail.com>
> 
> This approach looks like it works, though I still think adding an option
> for this under PTRACE_SETOPTIONS would be less intrusive.

Dave, thank you for the feedback. I will prepare a patch with an option
and then we will see what looks better.

> 
> Adding a shadow regset like this also looks like it would cause the gp
> regs to be pointlessly be dumped twice in a core dump.  Avoiding that
> might require hacks in the core code...
> 
> 
> > ---
> >  arch/arm64/kernel/ptrace.c | 39 ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/elf.h   |  1 +
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 1863f080cb07..b8e4c2ddf636 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -591,6 +591,15 @@ static int gpr_get(struct task_struct *target,
> >  	return ret;
> >  }
> >  
> > +static int gpr_get_full(struct task_struct *target,
> > +		   const struct user_regset *regset,
> > +		   struct membuf to)
> > +{
> > +	struct user_pt_regs *uregs = &task_pt_regs(target)->user_regs;
> > +
> > +	return membuf_write(&to, uregs, sizeof(*uregs));
> > +}
> > +
> >  static int gpr_set(struct task_struct *target, const struct user_regset *regset,
> >  		   unsigned int pos, unsigned int count,
> >  		   const void *kbuf, const void __user *ubuf)
> > @@ -1088,6 +1097,7 @@ static int tagged_addr_ctrl_set(struct task_struct *target, const struct
> >  
> >  enum aarch64_regset {
> >  	REGSET_GPR,
> > +	REGSET_GPR_FULL,
> 
> If we go with this approach, "REGSET_GPR_RAW" might be a preferable
> name.  Both regs represent all the regs ("full"), but REGSET_GPR is
> mangled by the kernel.

I agree that REGSET_GPR_RAW looks better in this case.

> 
> >  	REGSET_FPR,
> >  	REGSET_TLS,
> >  #ifdef CONFIG_HAVE_HW_BREAKPOINT
> > @@ -1119,6 +1129,14 @@ static const struct user_regset aarch64_regsets[] = {
> >  		.regset_get = gpr_get,
> >  		.set = gpr_set
> >  	},
> > +	[REGSET_GPR_FULL] = {
> > +		.core_note_type = NT_ARM_PRSTATUS,

...

> > diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> > index 30f68b42eeb5..a2086d19263a 100644
> > --- a/include/uapi/linux/elf.h
> > +++ b/include/uapi/linux/elf.h
> > @@ -426,6 +426,7 @@ typedef struct elf64_shdr {
> >  #define NT_ARM_PACA_KEYS	0x407	/* ARM pointer authentication address keys */
> >  #define NT_ARM_PACG_KEYS	0x408	/* ARM pointer authentication generic key */
> >  #define NT_ARM_TAGGED_ADDR_CTRL	0x409	/* arm64 tagged address control (prctl()) */
> 
> What happened to 0x40a..0x40f?

shame on me :)

> 
> [...]
> 
> Cheers
> ---Dave