All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: Andrei Vagin <avagin@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, 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
Date: Wed, 27 Jan 2021 14:53:07 +0000	[thread overview]
Message-ID: <20210127145304.GC13952@arm.com> (raw)
In-Reply-To: <20210119220637.494476-3-avagin@gmail.com>

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.

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.

>  	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,

Similarly, something like NT_ARM_PRSTATUS_RAW or similar.

> +		.n = sizeof(struct user_pt_regs) / sizeof(u64),
> +		.size = sizeof(u64),
> +		.align = sizeof(u64),
> +		.regset_get = gpr_get_full,
> +		.set = gpr_set
> +	},
>  	[REGSET_FPR] = {
>  		.core_note_type = NT_PRFPREG,
>  		.n = sizeof(struct user_fpsimd_state) / sizeof(u32),
> @@ -1225,6 +1243,7 @@ static const struct user_regset_view user_aarch64_view = {
>  #ifdef CONFIG_COMPAT
>  enum compat_regset {
>  	REGSET_COMPAT_GPR,
> +	REGSET_COMPAT_GPR_FULL,
>  	REGSET_COMPAT_VFP,
>  };
>  
> @@ -1285,6 +1304,18 @@ static int compat_gpr_get(struct task_struct *target,
>  	return 0;
>  }
>  
> +/* compat_gpr_get_full doesn't  overwrite x12 like compat_gpr_get. */
> +static int compat_gpr_get_full(struct task_struct *target,
> +			  const struct user_regset *regset,
> +			  struct membuf to)
> +{
> +	int i = 0;
> +
> +	while (to.left)
> +		membuf_store(&to, compat_get_user_reg(target, i++));
> +	return 0;
> +}
> +
>  static int compat_gpr_set(struct task_struct *target,
>  			  const struct user_regset *regset,
>  			  unsigned int pos, unsigned int count,
> @@ -1435,6 +1466,14 @@ static const struct user_regset aarch32_regsets[] = {
>  		.regset_get = compat_gpr_get,
>  		.set = compat_gpr_set
>  	},
> +	[REGSET_COMPAT_GPR_FULL] = {
> +		.core_note_type = NT_ARM_PRSTATUS,
> +		.n = COMPAT_ELF_NGREG,
> +		.size = sizeof(compat_elf_greg_t),
> +		.align = sizeof(compat_elf_greg_t),
> +		.regset_get = compat_gpr_get_full,
> +		.set = compat_gpr_set
> +	},
>  	[REGSET_COMPAT_VFP] = {
>  		.core_note_type = NT_ARM_VFP,
>  		.n = VFP_STATE_SIZE / sizeof(compat_ulong_t),
> 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?

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Dave Martin <Dave.Martin@arm.com>
To: Andrei Vagin <avagin@gmail.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
Date: Wed, 27 Jan 2021 14:53:07 +0000	[thread overview]
Message-ID: <20210127145304.GC13952@arm.com> (raw)
In-Reply-To: <20210119220637.494476-3-avagin@gmail.com>

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.

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.

>  	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,

Similarly, something like NT_ARM_PRSTATUS_RAW or similar.

> +		.n = sizeof(struct user_pt_regs) / sizeof(u64),
> +		.size = sizeof(u64),
> +		.align = sizeof(u64),
> +		.regset_get = gpr_get_full,
> +		.set = gpr_set
> +	},
>  	[REGSET_FPR] = {
>  		.core_note_type = NT_PRFPREG,
>  		.n = sizeof(struct user_fpsimd_state) / sizeof(u32),
> @@ -1225,6 +1243,7 @@ static const struct user_regset_view user_aarch64_view = {
>  #ifdef CONFIG_COMPAT
>  enum compat_regset {
>  	REGSET_COMPAT_GPR,
> +	REGSET_COMPAT_GPR_FULL,
>  	REGSET_COMPAT_VFP,
>  };
>  
> @@ -1285,6 +1304,18 @@ static int compat_gpr_get(struct task_struct *target,
>  	return 0;
>  }
>  
> +/* compat_gpr_get_full doesn't  overwrite x12 like compat_gpr_get. */
> +static int compat_gpr_get_full(struct task_struct *target,
> +			  const struct user_regset *regset,
> +			  struct membuf to)
> +{
> +	int i = 0;
> +
> +	while (to.left)
> +		membuf_store(&to, compat_get_user_reg(target, i++));
> +	return 0;
> +}
> +
>  static int compat_gpr_set(struct task_struct *target,
>  			  const struct user_regset *regset,
>  			  unsigned int pos, unsigned int count,
> @@ -1435,6 +1466,14 @@ static const struct user_regset aarch32_regsets[] = {
>  		.regset_get = compat_gpr_get,
>  		.set = compat_gpr_set
>  	},
> +	[REGSET_COMPAT_GPR_FULL] = {
> +		.core_note_type = NT_ARM_PRSTATUS,
> +		.n = COMPAT_ELF_NGREG,
> +		.size = sizeof(compat_elf_greg_t),
> +		.align = sizeof(compat_elf_greg_t),
> +		.regset_get = compat_gpr_get_full,
> +		.set = compat_gpr_set
> +	},
>  	[REGSET_COMPAT_VFP] = {
>  		.core_note_type = NT_ARM_VFP,
>  		.n = VFP_STATE_SIZE / sizeof(compat_ulong_t),
> 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?

[...]

Cheers
---Dave

  reply	other threads:[~2021-01-27 15:18 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-19 22:06 [PATCH 0/3] arm64/ptrace: allow to get all registers on syscall traps Andrei Vagin
2021-01-19 22:06 ` Andrei Vagin
2021-01-19 22:06 ` [PATCH 1/3] arm64/ptrace: don't clobber task registers on syscall entry/exit traps Andrei Vagin
2021-01-19 22:06   ` Andrei Vagin
2021-01-27 15:14   ` Dave Martin
2021-01-27 15:14     ` Dave Martin
2021-01-19 22:06 ` [PATCH 2/3] arm64/ptrace: introduce NT_ARM_PRSTATUS to get a full set of registers Andrei Vagin
2021-01-19 22:06   ` Andrei Vagin
2021-01-27 14:53   ` Dave Martin [this message]
2021-01-27 14:53     ` Dave Martin
2021-01-29  7:56     ` Andrei Vagin
2021-01-29  7:56       ` Andrei Vagin
2021-01-19 22:06 ` [PATCH 3/3] selftest/arm64/ptrace: add a test for NT_ARM_PRSTATUS Andrei Vagin
2021-01-19 22:06   ` Andrei Vagin
2021-01-27  8:10 ` [PATCH 0/3] arm64/ptrace: allow to get all registers on syscall traps Andrei Vagin
2021-01-27  8:10   ` Andrei Vagin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210127145304.GC13952@arm.com \
    --to=dave.martin@arm.com \
    --cc=avagin@gmail.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.