[PATCH 0/3] MIPS: DSP ASE regset support

[PATCH 0/3] MIPS: DSP ASE regset support

[Maciej W. Rozycki]

Hi,

 For years, quite oddly, we have been missing DSP ASE register state from 
core files.  These days regsets are used to define what goes into a core 
file, so here's a change adding one.

 As a side effect ptrace(2) can now also access this regset, however no 
complementing client implementation has been made.  Eventually that'll 
have to change though so that DSP ASE registers can be correctly accessed 
in n32 processes, which suffer from ptrace(2) 32-bit data types truncating 
contents exchanged by PTRACE_PEEKUSR and PTRACE_POKEUSR requests with 
64-bit registers and no means defined to access partial registers via this 
API.

 In the course of this implementation I came across two bugs affecting the 
area being updated and hence this has become a small patch series with the 
audience wider than originally expected.

 See individual commit descriptions for the details of changes made.  

 NB there is no strict functional dependency between 1/3 and 2/3-3/3, so 
the order of commits does not have to be preserved as far as these two 
subsets are concerned.  However 3/3 does trigger the problem addressed 
with 1/3 (and gracefully handles it), hence the grouping in a series.

 Please apply.

  Maciej

[PATCH 1/3] binfmt_elf: Respect error return from `regset->active'

[Maciej W. Rozycki]

The regset API documented in <linux/regset.h> defines -ENODEV as the 
result of the `->active' handler to be used where the feature requested 
is not available on the hardware found.  However code handling core file 
note generation in `fill_thread_core_info' interpretes any non-zero 
result from the `->active' handler as the regset requested being active. 
Consequently processing continues (and hopefully gracefully fails later 
on) rather than being abandoned right away for the regset requested.

Fix the problem then by making the code proceed only if a positive 
result is returned from the `->active' handler.

Cc: stable@vger.kernel.org # 2.6.25+
Fixes: 4206d3aa1978 ("elf core dump: notes user_regset")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
Hi,

 Overall we could also use the count returned by ->active to limit the 
size of data requested, i.e. something along the lines of:

	ssize_t size;

	if (regset->active)
		size = regset->active(t->task, regset);
	else
		size = regset_size(t->task, regset);
	if (size > 0) {
		...	

however that would be an optimisation that belongs to a separate change, 
which (due to the need to unload stuff I have in progress already) I am 
not going to make at this point.  Perhaps someone else would be willing 
to pick up the idea.

 Anyway, please apply.

  Maciej
---
 fs/binfmt_elf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-elf-core-regset-active.diff
Index: linux-jhogan-test/fs/binfmt_elf.c
===================================================================
--- linux-jhogan-test.orig/fs/binfmt_elf.c	2018-03-21 17:14:55.000000000 +0000
+++ linux-jhogan-test/fs/binfmt_elf.c	2018-05-09 23:25:50.742255000 +0100
@@ -1739,7 +1739,7 @@ static int fill_thread_core_info(struct 
 		const struct user_regset *regset = &view->regsets[i];
 		do_thread_regset_writeback(t->task, regset);
 		if (regset->core_note_type && regset->get &&
-		    (!regset->active || regset->active(t->task, regset))) {
+		    (!regset->active || regset->active(t->task, regset) > 0)) {
 			int ret;
 			size_t size = regset_size(t->task, regset);
 			void *data = kmalloc(size, GFP_KERNEL);

Re: [PATCH 1/3] binfmt_elf: Respect error return from `regset->active'

[Paul Burton]

Hi Alexander,

On Tue, May 15, 2018 at 11:32:45PM +0100, Maciej W. Rozycki wrote:
> The regset API documented in <linux/regset.h> defines -ENODEV as the 
> result of the `->active' handler to be used where the feature requested 
> is not available on the hardware found.  However code handling core file 
> note generation in `fill_thread_core_info' interpretes any non-zero 
> result from the `->active' handler as the regset requested being active. 
> Consequently processing continues (and hopefully gracefully fails later 
> on) rather than being abandoned right away for the regset requested.
> 
> Fix the problem then by making the code proceed only if a positive 
> result is returned from the `->active' handler.
> 
> Cc: stable@vger.kernel.org # 2.6.25+
> Fixes: 4206d3aa1978 ("elf core dump: notes user_regset")
> Signed-off-by: Maciej W. Rozycki <macro@mips.com>

<snip>

> --- linux-jhogan-test.orig/fs/binfmt_elf.c	2018-03-21 17:14:55.000000000 +0000
> +++ linux-jhogan-test/fs/binfmt_elf.c	2018-05-09 23:25:50.742255000 +0100
> @@ -1739,7 +1739,7 @@ static int fill_thread_core_info(struct 
>  		const struct user_regset *regset = &view->regsets[i];
>  		do_thread_regset_writeback(t->task, regset);
>  		if (regset->core_note_type && regset->get &&
> -		    (!regset->active || regset->active(t->task, regset))) {
> +		    (!regset->active || regset->active(t->task, regset) > 0)) {
>  			int ret;
>  			size_t size = regset_size(t->task, regset);
>  			void *data = kmalloc(size, GFP_KERNEL);
> 

This looks obviously right to me, although I don't think it affects
anything until commit 25847fb195ae ("powerpc/ptrace: Enable support for
NT_PPC_CGPR") in v4.8-rc1 & even then not in a harmful way so I'd drop
the stable tag.

You show up as maintainer for fs/binfmt_elf.c though, so before I go
applying this to mips-next does it look good to you?

Thanks,
    Paul

Re: [PATCH 1/3] binfmt_elf: Respect error return from `regset->active'

[Maciej W. Rozycki]

Hi Paul,

> > --- linux-jhogan-test.orig/fs/binfmt_elf.c	2018-03-21 17:14:55.000000000 +0000
> > +++ linux-jhogan-test/fs/binfmt_elf.c	2018-05-09 23:25:50.742255000 +0100
> > @@ -1739,7 +1739,7 @@ static int fill_thread_core_info(struct 
> >  		const struct user_regset *regset = &view->regsets[i];
> >  		do_thread_regset_writeback(t->task, regset);
> >  		if (regset->core_note_type && regset->get &&
> > -		    (!regset->active || regset->active(t->task, regset))) {
> > +		    (!regset->active || regset->active(t->task, regset) > 0)) {
> >  			int ret;
> >  			size_t size = regset_size(t->task, regset);
> >  			void *data = kmalloc(size, GFP_KERNEL);
> > 
> 
> This looks obviously right to me, although I don't think it affects
> anything until commit 25847fb195ae ("powerpc/ptrace: Enable support for
> NT_PPC_CGPR") in v4.8-rc1 & even then not in a harmful way so I'd drop
> the stable tag.

 I'm fine with dropping the tag FWIW.  Thanks for taking care of my patch.

  Maciej

[PATCH 2/3] MIPS: Correct the 64-bit DSP accumulator register size

[Maciej W. Rozycki]

Use the `unsigned long' rather than `__u32' type for DSP accumulator 
registers, like with the regular MIPS multiply/divide accumulator and 
general-purpose registers, as all are 64-bit in 64-bit implementations 
and using a 32-bit data type leads to contents truncation on context 
saving.

Update `arch_ptrace' and `compat_arch_ptrace' accordingly, removing 
casts that are similarly not used with multiply/divide accumulator or 
general-purpose register accesses.

Cc: stable@vger.kernel.org # 2.6.15+
Fixes: e50c0a8fa60d ("Support the MIPS32 / MIPS64 DSP ASE.")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
Hi,

 I have no 64-bit DSP hardware handy to verify this change, however some 
surely exists and is used to run Linux, as indicated by GDB PR gdb/22286, 
<https://sourceware.org/bugzilla/show_bug.cgi?id=22286>, so we better get 
it right before people start screaming.

  Maciej
---
 arch/mips/include/asm/processor.h |    2 +-
 arch/mips/kernel/ptrace.c         |    2 +-
 arch/mips/kernel/ptrace32.c       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

linux-mips-dsp64.diff
Index: linux-jhogan-test/arch/mips/include/asm/processor.h
===================================================================
--- linux-jhogan-test.orig/arch/mips/include/asm/processor.h	2018-03-21 17:13:52.000000000 +0000
+++ linux-jhogan-test/arch/mips/include/asm/processor.h	2018-05-09 22:35:33.248559000 +0100
@@ -141,7 +141,7 @@ struct mips_fpu_struct {
 
 #define NUM_DSP_REGS   6
 
-typedef __u32 dspreg_t;
+typedef unsigned long dspreg_t;
 
 struct mips_dsp_state {
 	dspreg_t	dspr[NUM_DSP_REGS];
Index: linux-jhogan-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-jhogan-test.orig/arch/mips/kernel/ptrace.c	2018-05-09 22:34:00.000000000 +0100
+++ linux-jhogan-test/arch/mips/kernel/ptrace.c	2018-05-09 22:37:45.416608000 +0100
@@ -856,7 +856,7 @@ long arch_ptrace(struct task_struct *chi
 				goto out;
 			}
 			dregs = __get_dsp_regs(child);
-			tmp = (unsigned long) (dregs[addr - DSP_BASE]);
+			tmp = dregs[addr - DSP_BASE];
 			break;
 		}
 		case DSP_CONTROL:
Index: linux-jhogan-test/arch/mips/kernel/ptrace32.c
===================================================================
--- linux-jhogan-test.orig/arch/mips/kernel/ptrace32.c	2018-03-21 17:13:52.000000000 +0000
+++ linux-jhogan-test/arch/mips/kernel/ptrace32.c	2018-05-09 22:45:50.924418000 +0100
@@ -142,7 +142,7 @@ long compat_arch_ptrace(struct task_stru
 				goto out;
 			}
 			dregs = __get_dsp_regs(child);
-			tmp = (unsigned long) (dregs[addr - DSP_BASE]);
+			tmp = dregs[addr - DSP_BASE];
 			break;
 		}
 		case DSP_CONTROL:

[PATCH 3/3] MIPS: Add DSP ASE regset support

[Maciej W. Rozycki]

Define an NT_MIPS_DSP core file note type and implement a corresponding 
regset holding the DSP ASE register context, following the layout of the 
`mips_dsp_state' structure, except for the DSPControl register stored as 
a 64-bit rather than 32-bit quantity in a 64-bit note.

The lack of DSP ASE register saving to core files can be considered a 
design flaw with commit e50c0a8fa60d ("Support the MIPS32 / MIPS64 DSP 
ASE."), leading to an incomplete state being saved.  Consequently no DSP 
ASE regset has been created with commit 7aeb753b5353 ("MIPS: Implement 
task_user_regset_view."), when regset support was added to the MIPS 
port.

Additionally there is no way for ptrace(2) to correctly access the DSP 
accumulator registers in n32 processes with the existing interfaces.  
This is due to 32-bit truncation of data passed with PTRACE_PEEKUSR and 
PTRACE_POKEUSR requests, which cannot be avoided owing to how the data 
types for ptrace(3) have been defined.  This new NT_MIPS_DSP regset 
fills the missing interface gap.

Cc: <stable@vger.kernel.org> # 3.13+
Fixes: 7aeb753b5353 ("MIPS: Implement task_user_regset_view.")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
Hi,

 This was verified with 32-bit DSP and non-DSP hardware configurations by 
dumping cores and examining, with `readelf', the notes created.  In the 
former case DSP registers were filled with patterns by the program being 
crashed and the patterns verified in the core file produced.  I have no 
64-bit DSP hardware handy, but the same code has been used, except for the 
data type holding register data, for both 32-bit and 64-bit notes, so it 
should be obviously correct.

 As noted in the commit description I consider it a design flaw and 
therefore I think it makes sense to backport this change and propose doing 
so.

  Maciej 
---
 arch/mips/kernel/ptrace.c |  189 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/elf.h  |    1 
 2 files changed, 190 insertions(+)

linux-mips-regset-dsp.diff
Index: linux-jhogan-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-jhogan-test.orig/arch/mips/kernel/ptrace.c	2018-05-09 23:26:36.787614000 +0100
+++ linux-jhogan-test/arch/mips/kernel/ptrace.c	2018-05-09 23:53:18.864657000 +0100
@@ -41,6 +41,7 @@
 #include <asm/mipsmtregs.h>
 #include <asm/pgtable.h>
 #include <asm/page.h>
+#include <asm/processor.h>
 #include <asm/syscall.h>
 #include <linux/uaccess.h>
 #include <asm/bootinfo.h>
@@ -589,9 +590,179 @@ static int fpr_set(struct task_struct *t
 	return err;
 }
 
+#if defined(CONFIG_32BIT) || defined(CONFIG_MIPS32_O32)
+
+/*
+ * Copy the DSP context to the supplied 32-bit NT_MIPS_DSP buffer.
+ */
+static int dsp32_get(struct task_struct *target,
+		     const struct user_regset *regset,
+		     unsigned int pos, unsigned int count,
+		     void *kbuf, void __user *ubuf)
+{
+	unsigned int start, num_regs, i;
+	u32 dspregs[NUM_DSP_REGS + 1];
+
+	BUG_ON(count % sizeof(u32));
+
+	if (!cpu_has_dsp)
+		return -EIO;
+
+	start = pos / sizeof(u32);
+	num_regs = count / sizeof(u32);
+
+	if (start + num_regs > NUM_DSP_REGS + 1)
+		return -EIO;
+
+	for (i = start; i < num_regs; i++)
+		switch (i) {
+		case 0 ... NUM_DSP_REGS - 1:
+			dspregs[i] = target->thread.dsp.dspr[i];
+			break;
+		case NUM_DSP_REGS:
+			dspregs[i] = target->thread.dsp.dspcontrol;
+			break;
+		}
+	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, dspregs, 0,
+				   sizeof(dspregs));
+}
+
+/*
+ * Copy the supplied 32-bit NT_MIPS_DSP buffer to the DSP context.
+ */
+static int dsp32_set(struct task_struct *target,
+		     const struct user_regset *regset,
+		     unsigned int pos, unsigned int count,
+		     const void *kbuf, const void __user *ubuf)
+{
+	unsigned int start, num_regs, i;
+	u32 dspregs[NUM_DSP_REGS + 1];
+	int err;
+
+	BUG_ON(count % sizeof(u32));
+
+	if (!cpu_has_dsp)
+		return -EIO;
+
+	start = pos / sizeof(u32);
+	num_regs = count / sizeof(u32);
+
+	if (start + num_regs > NUM_DSP_REGS + 1)
+		return -EIO;
+
+	err = user_regset_copyin(&pos, &count, &kbuf, &ubuf, dspregs, 0,
+				 sizeof(dspregs));
+	if (err)
+		return err;
+
+	for (i = start; i < num_regs; i++)
+		switch (i) {
+		case 0 ... NUM_DSP_REGS - 1:
+			target->thread.dsp.dspr[i] = (s32)dspregs[i];
+			break;
+		case NUM_DSP_REGS:
+			target->thread.dsp.dspcontrol = (s32)dspregs[i];
+			break;
+		}
+
+	return 0;
+}
+
+#endif /* CONFIG_32BIT || CONFIG_MIPS32_O32 */
+
+#ifdef CONFIG_64BIT
+
+/*
+ * Copy the DSP context to the supplied 64-bit NT_MIPS_DSP buffer.
+ */
+static int dsp64_get(struct task_struct *target,
+		     const struct user_regset *regset,
+		     unsigned int pos, unsigned int count,
+		     void *kbuf, void __user *ubuf)
+{
+	unsigned int start, num_regs, i;
+	u64 dspregs[NUM_DSP_REGS + 1];
+
+	BUG_ON(count % sizeof(u64));
+
+	if (!cpu_has_dsp)
+		return -EIO;
+
+	start = pos / sizeof(u64);
+	num_regs = count / sizeof(u64);
+
+	if (start + num_regs > NUM_DSP_REGS + 1)
+		return -EIO;
+
+	for (i = start; i < num_regs; i++)
+		switch (i) {
+		case 0 ... NUM_DSP_REGS - 1:
+			dspregs[i] = target->thread.dsp.dspr[i];
+			break;
+		case NUM_DSP_REGS:
+			dspregs[i] = target->thread.dsp.dspcontrol;
+			break;
+		}
+	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, dspregs, 0,
+				   sizeof(dspregs));
+}
+
+/*
+ * Copy the supplied 64-bit NT_MIPS_DSP buffer to the DSP context.
+ */
+static int dsp64_set(struct task_struct *target,
+		     const struct user_regset *regset,
+		     unsigned int pos, unsigned int count,
+		     const void *kbuf, const void __user *ubuf)
+{
+	unsigned int start, num_regs, i;
+	u64 dspregs[NUM_DSP_REGS + 1];
+	int err;
+
+	BUG_ON(count % sizeof(u64));
+
+	if (!cpu_has_dsp)
+		return -EIO;
+
+	start = pos / sizeof(u64);
+	num_regs = count / sizeof(u64);
+
+	if (start + num_regs > NUM_DSP_REGS + 1)
+		return -EIO;
+
+	err = user_regset_copyin(&pos, &count, &kbuf, &ubuf, dspregs, 0,
+				 sizeof(dspregs));
+	if (err)
+		return err;
+
+	for (i = start; i < num_regs; i++)
+		switch (i) {
+		case 0 ... NUM_DSP_REGS - 1:
+			target->thread.dsp.dspr[i] = dspregs[i];
+			break;
+		case NUM_DSP_REGS:
+			target->thread.dsp.dspcontrol = dspregs[i];
+			break;
+		}
+
+	return 0;
+}
+
+#endif /* CONFIG_64BIT */
+
+/*
+ * Determine whether the DSP context is present.
+ */
+static int dsp_active(struct task_struct *target,
+		      const struct user_regset *regset)
+{
+	return cpu_has_dsp ? NUM_DSP_REGS + 1 : -ENODEV;
+}
+
 enum mips_regset {
 	REGSET_GPR,
 	REGSET_FPR,
+	REGSET_DSP,
 };
 
 struct pt_regs_offset {
@@ -697,6 +868,15 @@ static const struct user_regset mips_reg
 		.get		= fpr_get,
 		.set		= fpr_set,
 	},
+	[REGSET_DSP] = {
+		.core_note_type	= NT_MIPS_DSP,
+		.n		= NUM_DSP_REGS + 1,
+		.size		= sizeof(u32),
+		.align		= sizeof(u32),
+		.get		= dsp32_get,
+		.set		= dsp32_set,
+		.active		= dsp_active,
+	},
 };
 
 static const struct user_regset_view user_mips_view = {
@@ -728,6 +908,15 @@ static const struct user_regset mips64_r
 		.get		= fpr_get,
 		.set		= fpr_set,
 	},
+	[REGSET_DSP] = {
+		.core_note_type	= NT_MIPS_DSP,
+		.n		= NUM_DSP_REGS + 1,
+		.size		= sizeof(u64),
+		.align		= sizeof(u64),
+		.get		= dsp64_get,
+		.set		= dsp64_set,
+		.active		= dsp_active,
+	},
 };
 
 static const struct user_regset_view user_mips64_view = {
Index: linux-jhogan-test/include/uapi/linux/elf.h
===================================================================
--- linux-jhogan-test.orig/include/uapi/linux/elf.h	2018-05-09 23:22:44.799797000 +0100
+++ linux-jhogan-test/include/uapi/linux/elf.h	2018-05-09 23:51:32.646880000 +0100
@@ -424,6 +424,7 @@ typedef struct elf64_shdr {
 #define NT_METAG_RPIPE	0x501		/* Metag read pipeline state */
 #define NT_METAG_TLS	0x502		/* Metag TLS pointer */
 #define NT_ARC_V2	0x600		/* ARCv2 accumulator/extra registers */
+#define NT_MIPS_DSP	0x700		/* MIPS DSP ASE registers */
 
 /* Note header in a PT_NOTE section */
 typedef struct elf32_note {

Re: [PATCH 0/3] MIPS: DSP ASE regset support

[Paul Burton]

Hi Maciej,

On Tue, May 15, 2018 at 11:32:11PM +0100, Maciej W. Rozycki wrote:
>  For years, quite oddly, we have been missing DSP ASE register state from 
> core files.  These days regsets are used to define what goes into a core 
> file, so here's a change adding one.
> 
>  As a side effect ptrace(2) can now also access this regset, however no 
> complementing client implementation has been made.  Eventually that'll 
> have to change though so that DSP ASE registers can be correctly accessed 
> in n32 processes, which suffer from ptrace(2) 32-bit data types truncating 
> contents exchanged by PTRACE_PEEKUSR and PTRACE_POKEUSR requests with 
> 64-bit registers and no means defined to access partial registers via this 
> API.
> 
>  In the course of this implementation I came across two bugs affecting the 
> area being updated and hence this has become a small patch series with the 
> audience wider than originally expected.
> 
>  See individual commit descriptions for the details of changes made.  
> 
>  NB there is no strict functional dependency between 1/3 and 2/3-3/3, so 
> the order of commits does not have to be preserved as far as these two 
> subsets are concerned.  However 3/3 does trigger the problem addressed 
> with 1/3 (and gracefully handles it), hence the grouping in a series.
> 
>  Please apply.

Thanks - applied to mips-next for 4.19, though I removed the stable tags
on patches 1/3 & 3/3 & it's worth noting that the ELF note numbers are
changed to from 0x70X to 0x80X, since 0x700 has been used already.

If you really care about a stable backport for 3/3 let's talk, but I'm
doubtful as to its use when we've been missing this for so long.

Paul

Re: [PATCH 0/3] MIPS: DSP ASE regset support

[Maciej W. Rozycki]

Hi Paul,

> Thanks - applied to mips-next for 4.19, though I removed the stable tags
> on patches 1/3 & 3/3 & it's worth noting that the ELF note numbers are
> changed to from 0x70X to 0x80X, since 0x700 has been used already.

 Great, thanks!  No code has used the note numbers yet as I ran out of 
time for doing the GDB side, so the change is not going to affect 
anything.

> If you really care about a stable backport for 3/3 let's talk, but I'm
> doubtful as to its use when we've been missing this for so long.

 I guess it can be backported if a need arises.  There's been clearly some 
move on the 64-bit DSP side as indicated by this GDB bug report (mentioned 
previously): <https://sourceware.org/bugzilla/show_bug.cgi?id=22286>, 
which ultimately prompted me to write this patch series, however given 
that GDB has also been broken for years it looks like just the beginning.
So maybe people will cope with the requirement to upgrade.  They need to 
notice the missing GDB part first anyway. ;)

  Maciej