On Thu, Mar 14, 2024 at 04:53:28PM +0530, Vignesh Balasubramanian wrote:
> Add a new .note section containing type, size, offset and flags of
> every xfeature that is present.
>
> This information will be used by the debuggers to understand the XSAVE
> layout of the machine where the core file is dumped, and to read XSAVE
> registers, especially during cross-platform debugging.
I see binutils in CC. Can someone from gdb confirm that this solution
can be used?
>
> Co-developed-by: Jini Susan George <jinisusan.geo...@amd.com>
> Signed-off-by: Jini Susan George <jinisusan.geo...@amd.com>
> Signed-off-by: Vignesh Balasubramanian <vigba...@amd.com>
> ---
> arch/Kconfig | 9 +++
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/elf.h | 2 -
> arch/x86/Kconfig | 1 +
> arch/x86/include/asm/elf.h | 7 +++
> arch/x86/kernel/fpu/xstate.c | 101 +++++++++++++++++++++++++++++++++
> include/linux/elf.h | 2 +-
> include/uapi/linux/elf.h | 1 +
> 8 files changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index fd18b7db2c77..3bd8a0b2bba1 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -502,6 +502,15 @@ config MMU_LAZY_TLB_SHOOTDOWN
> config ARCH_HAVE_NMI_SAFE_CMPXCHG
> bool
>
> +config ARCH_HAVE_EXTRA_ELF_NOTES
> + bool
> + help
> + An architecture should select this in order to enable adding an
> + arch-specific ELF note section to core files. It must provide two
> + functions: elf_coredump_extra_notes_size() and
> + elf_coredump_extra_notes_write() which are invoked by the ELF core
> + dumper.
> +
> config ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> bool
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index a91cb070ca4a..3b31bd7490e2 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -156,6 +156,7 @@ config PPC
> select ARCH_HAS_UACCESS_FLUSHCACHE
> select ARCH_HAS_UBSAN
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> + select ARCH_HAVE_EXTRA_ELF_NOTES if SPU_BASE
> select ARCH_KEEP_MEMBLOCK
> select ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE if PPC_RADIX_MMU
> select ARCH_MIGHT_HAVE_PC_PARPORT
> diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
> index 79f1c480b5eb..bb4b94444d3e 100644
> --- a/arch/powerpc/include/asm/elf.h
> +++ b/arch/powerpc/include/asm/elf.h
> @@ -127,8 +127,6 @@ extern int arch_setup_additional_pages(struct
> linux_binprm *bprm,
> /* Notes used in ET_CORE. Note name is "SPU/<fd>/<filename>". */
> #define NT_SPU 1
>
> -#define ARCH_HAVE_EXTRA_ELF_NOTES
> -
> #endif /* CONFIG_SPU_BASE */
>
> #ifdef CONFIG_PPC64
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 78050d5d7fac..35e8d1201099 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -104,6 +104,7 @@ config X86
> select ARCH_HAS_DEBUG_WX
> select ARCH_HAS_ZONE_DMA_SET if EXPERT
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> + select ARCH_HAVE_EXTRA_ELF_NOTES
> select ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
> select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
> select ARCH_MIGHT_HAVE_PC_PARPORT
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 1fb83d47711f..1b9f0b4bf6bc 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -13,6 +13,13 @@
> #include <asm/auxvec.h>
> #include <asm/fsgsbase.h>
>
> +struct xfeat_component {
> + u32 xfeat_type;
> + u32 xfeat_sz;
> + u32 xfeat_off;
> + u32 xfeat_flags;
> +} __packed;
While it is currently true, just for robustness, can you add
a _Static_assert that sizeof(struct xfeat_component) % 4 == 0 ?
Notes must be 4-byte aligned.
> +
> typedef unsigned long elf_greg_t;
>
> #define ELF_NGREG (sizeof(struct user_regs_struct) / sizeof(elf_greg_t))
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 117e74c44e75..6e5ea483ec1d 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -13,6 +13,7 @@
> #include <linux/seq_file.h>
> #include <linux/proc_fs.h>
> #include <linux/vmalloc.h>
> +#include <linux/coredump.h>
>
> #include <asm/fpu/api.h>
> #include <asm/fpu/regset.h>
> @@ -1836,3 +1837,103 @@ int proc_pid_arch_status(struct seq_file *m, struct
> pid_namespace *ns,
> return 0;
> }
> #endif /* CONFIG_PROC_PID_ARCH_STATUS */
> +
> +/*
> + * Dump type, size, offset and flag values for every xfeature that is
> present.
> + */
> +static int dump_xsave_layout_desc(struct coredump_params *cprm)
> +{
> +
> + struct xfeat_component xc;
> + int num_records = 0;
> + int i;
> +
> + /* XFEATURE_FPU and XFEATURE_SSE, both are fixed legacy states. */
> + for (i = 0; i < FIRST_EXTENDED_XFEATURE; i++) {
> + xc.xfeat_type = i;
> + xc.xfeat_sz = xstate_sizes[i];
> + xc.xfeat_off = xstate_offsets[i];
> + xc.xfeat_flags = xstate_flags[i];
> +
> + if (!dump_emit(cprm, &xc, sizeof(struct xfeat_component)))
> + return 0;
> + num_records++;
> + }
> +
> + for_each_extended_xfeature(i, fpu_user_cfg.max_features) {
> + xc.xfeat_type = i;
> + xc.xfeat_sz = xstate_sizes[i];
> + xc.xfeat_off = xstate_offsets[i];
> + xc.xfeat_flags = xstate_flags[i];
> +
> + if (!dump_emit(cprm, &xc, sizeof(struct xfeat_component)))
> + return 0;
> + num_records++;
> + }
> +
> + return num_records;
> +}
> +
> +static int get_xsave_desc_size(void)
> +{
> + /* XFEATURE_FP and XFEATURE_SSE, both are fixed legacy states */
> + int xfeatures_count = 2;
> + int i;
> +
> + for_each_extended_xfeature(i, fpu_user_cfg.max_features)
> + xfeatures_count++;
> +
> + return xfeatures_count * (sizeof(struct xfeat_component));
> +}
> +
> +int elf_coredump_extra_notes_write(struct coredump_params *cprm)
> +{
> + const char *owner_name = "LINUX";
If you use an array instead of a pointer, there's no need for strlen(),
and you can make it a static outside of the function to refer to it
later.
static const char owner_name[] = "LINUX";
> + int num_records = 0;
> + struct elf_note en;
> +
> + en.n_namesz = strlen(owner_name) + 1;
en.n_namesz = sizeof(owner_name);
> + en.n_descsz = get_xsave_desc_size();
> + en.n_type = NT_X86_XSAVE_LAYOUT;
> +
> + if (!dump_emit(cprm, &en, sizeof(en)))
> + return 1;
> + if (!dump_emit(cprm, owner_name, en.n_namesz))
> + return 1;
> + if (!dump_align(cprm, 4))
> + return 1;
> +
> + num_records = dump_xsave_layout_desc(cprm);
> + if (!num_records) {
> + pr_warn("Error adding XSTATE layout ELF note. XSTATE buffer in
> the core file will be unparseable.");
Can you make this pr_warn_ratelimited() (and below)?
> + return 1;
> + }
> +
> + /* Total size should be equal to the number of records */
> + if ((sizeof(struct xfeat_component) * num_records) != en.n_descsz) {
> + pr_warn("Error adding XSTATE layout ELF note. The size of the
> .note section does not match with the total size of the records.");
> + return 1;
> + }
> +
> + if (!dump_align(cprm, 4))
> + return 1;
I don't think this call is needed?
> +
> + return 0;
> +}
> +
> +/*
> + * Return the size of new note.
> + */
> +int elf_coredump_extra_notes_size(void)
> +{
> + const char *fullname = "LINUX";
Now this can be dropped.
> + int size = 0;
> +
> + /* NOTE Header */
> + size += sizeof(struct elf_note);
> + /* name + align */
> + size += roundup(strlen(fullname) + 1, 4);
size += roundup(sizeof(owner_name), 4);
> + size += get_xsave_desc_size();
> +
> + return size;
> +}
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index c9a46c4e183b..5c402788da19 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -65,7 +65,7 @@ extern Elf64_Dyn _DYNAMIC [];
> struct file;
> struct coredump_params;
>
> -#ifndef ARCH_HAVE_EXTRA_ELF_NOTES
> +#ifndef CONFIG_ARCH_HAVE_EXTRA_ELF_NOTES
> static inline int elf_coredump_extra_notes_size(void) { return 0; }
> static inline int elf_coredump_extra_notes_write(struct coredump_params
> *cprm) { return 0; }
> #else
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 9417309b7230..3325488cb39b 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -411,6 +411,7 @@ typedef struct elf64_shdr {
> #define NT_X86_XSTATE 0x202 /* x86 extended state using
> xsave */
> /* Old binutils treats 0x203 as a CET state */
> #define NT_X86_SHSTK 0x204 /* x86 SHSTK state */
> +#define NT_X86_XSAVE_LAYOUT 0x205 /* XSAVE layout description */
> #define NT_S390_HIGH_GPRS 0x300 /* s390 upper register halves */
> #define NT_S390_TIMER 0x301 /* s390 timer register */
> #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator
> register */
> --
> 2.43.0
>
Otherwise looks reasonable, though I see Dave has feedback to address
too. :)
Thanks for working on this!
-Kees
--
Kees Cook
