re: [PATCH v3] Include XSTATE note in x86 core dumps

[matthew green]

> Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep
> API for injecting per-LWP notes into coredumps, and use it to append
> PT_GETXSTATE note.

this macro is pretty gross.  these are the problems i see:

- assumes 'int error'
- assumes 'struct lwp *l = curlwp'
- assumes 'name' is something
- assumes 'ns' is something
- returns from macro

all of those should be resolved.  4 are simple, they can be easy
inputs.  the return issue is more difficult, and perhaps should
be revisited (ie, perhaps just skip over this entry instead of
assuming return is right.)

> Since the XSTATE block uses the same format on i386 and amd64, the code
> does not have to conditionalize between 32-bit and 64-bit ELF format
> on that.  However, it does need to distinguish between 32-bit and 64-bit
> PT_* values.  In order to do that, it reuses PT32_* constant already
> present for ptrace(), and adds a matching PT64_GETXSTATE to satisfy
> the cpp logic.

i don't understand why the need to include netbsd32 headers or
define these separately.

if the layout is the same, what's the benefit?  we already know
to assume 32 or 64 bit from the ELF core header, right?


.mrg.

Re: [PATCH v2] Include XSTATE note in x86 core dumps

[Michał Górny]

On Sun, 2019-07-14 at 16:17 +0200, Maxime Villard wrote:
> Le 05/07/2019 à 17:22, Michał Górny a écrit :
> > +#ifdef EXEC_ELF32
> > +#include 
> > +#endif
> > +#define PT64_GETXSTATE PT_GETXSTATE
> > +#define COREDUMP_MACHDEP_LWP_NOTES \
> > +{  \
> > +   struct xstate xstate;   \
> 
> memset needed, otherwise we leak stuff

Good catch, thanks.  I've sent v3 to the ml but accidentally forgot to
set in-reply-to.

> 
> > +   error = process_read_xstate(l, );\
> > +   if (error)  \
> > +   return (error); \
> > +   ELFNAMEEND(coredump_savenote)(ns,   \
> > +   CONCAT(CONCAT(PT, ELFSIZE), _GETXSTATE), name, , \
> > +   sizeof(xstate));\
> > +}

-- 
Best regards,
Michał Górny



signature.asc
Description: This is a digitally signed message part

[PATCH v3] Include XSTATE note in x86 core dumps

[Michał Górny]

Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep
API for injecting per-LWP notes into coredumps, and use it to append
PT_GETXSTATE note.

Since the XSTATE block uses the same format on i386 and amd64, the code
does not have to conditionalize between 32-bit and 64-bit ELF format
on that.  However, it does need to distinguish between 32-bit and 64-bit
PT_* values.  In order to do that, it reuses PT32_* constant already
present for ptrace(), and adds a matching PT64_GETXSTATE to satisfy
the cpp logic.
---
 sys/arch/amd64/include/ptrace.h | 16 
 sys/arch/i386/include/ptrace.h  | 11 +++
 sys/kern/core_elf32.c   |  6 +-
 3 files changed, 32 insertions(+), 1 deletion(-)

/* CHANGED in v3:
 * added memset() on xstate to prevent leaking data
 */

diff --git a/sys/arch/amd64/include/ptrace.h b/sys/arch/amd64/include/ptrace.h
index 44b38d750f9c..a8338b32fa58 100644
--- a/sys/arch/amd64/include/ptrace.h
+++ b/sys/arch/amd64/include/ptrace.h
@@ -90,6 +90,22 @@
 int process_machdep_doxstate(struct lwp *, struct lwp *, struct uio *);
 int process_machdep_validxstate(struct proc *);
 
+#ifdef EXEC_ELF32
+#include 
+#endif
+#define PT64_GETXSTATE PT_GETXSTATE
+#define COREDUMP_MACHDEP_LWP_NOTES \
+{  \
+   struct xstate xstate;   \
+   memset(, 0, sizeof(xstate)); \
+   error = process_read_xstate(l, );\
+   if (error)  \
+   return (error); \
+   ELFNAMEEND(coredump_savenote)(ns,   \
+   CONCAT(CONCAT(PT, ELFSIZE), _GETXSTATE), name, , \
+   sizeof(xstate));\
+}
+
 #endif /* _KERNEL */
 
 #ifdef _KERNEL_OPT
diff --git a/sys/arch/i386/include/ptrace.h b/sys/arch/i386/include/ptrace.h
index 41ad7a119f0b..c9ec102a12dc 100644
--- a/sys/arch/i386/include/ptrace.h
+++ b/sys/arch/i386/include/ptrace.h
@@ -159,6 +159,17 @@
{ DT_REG, N("xmmregs"), Pmachdep_xmmregs,   \
  procfs_machdep_validxmmregs },
 
+#define COREDUMP_MACHDEP_LWP_NOTES \
+{  \
+   struct xstate xstate;   \
+   memset(, 0, sizeof(xstate)); \
+   error = process_read_xstate(l, );\
+   if (error)  \
+   return (error); \
+   ELFNAMEEND(coredump_savenote)(ns, PT_GETXSTATE, name, ,  \
+   sizeof(xstate));\
+}
+
 struct xmmregs;
 
 /* Functions used by both ptrace(2) and procfs. */
diff --git a/sys/kern/core_elf32.c b/sys/kern/core_elf32.c
index 5b69cab2ab74..ce90a1e44fa9 100644
--- a/sys/kern/core_elf32.c
+++ b/sys/kern/core_elf32.c
@@ -500,7 +500,11 @@ ELFNAMEEND(coredump_note)(struct lwp *l, struct note_state 
*ns)
 
ELFNAMEEND(coredump_savenote)(ns, PT_GETFPREGS, name, , freglen);
 #endif
-   /* XXX Add hook for machdep per-LWP notes. */
+
+#ifdef COREDUMP_MACHDEP_LWP_NOTES
+   COREDUMP_MACHDEP_LWP_NOTES
+#endif
+
return (0);
 }
 
-- 
2.22.0