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
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: [email protected] # 2.6.15+
Fixes: e50c0a8fa60d ("Support the MIPS32 / MIPS64 DSP ASE.")
Signed-off-by: Maciej W. Rozycki <[email protected]>
---
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:
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: <[email protected]> # 3.13+
Fixes: 7aeb753b5353 ("MIPS: Implement task_user_regset_view.")
Signed-off-by: Maciej W. Rozycki <[email protected]>
---
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 {
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: [email protected] # 2.6.25+
Fixes: 4206d3aa1978 ("elf core dump: notes user_regset")
Signed-off-by: Maciej W. Rozycki <[email protected]>
---
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);
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: [email protected] # 2.6.25+
> Fixes: 4206d3aa1978 ("elf core dump: notes user_regset")
> Signed-off-by: Maciej W. Rozycki <[email protected]>
<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
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
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
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