2019-12-02 18:04:21

by Omar Sandoval

[permalink] [raw]
Subject: [PATCH] x86: define arch_crash_save_vmcoreinfo() if CONFIG_CRASH_CORE=y

From: Omar Sandoval <[email protected]>

kernel/crash_core.c calls arch_crash_save_vmcoreinfo() to get
arch-specific bits for vmcoreinfo. If it is not defined, then it has a
no-op fallback. kernel/crash_core.c is gated behind CONFIG_CRASH_CORE.
However, x86 defines arch_crash_save_vmcoreinfo() in
arch/x86/kernel/machine_kexec_*.c, which is gated behind
CONFIG_KEXEC_CORE. So, a kernel with CONFIG_CRASH_CORE=y and
CONFIG_KEXEC_CORE=n uses the fallback and gets incomplete vmcoreinfo
data, which can confuse applications reading vmcoreinfo from
/proc/kcore.

Fix it by moving arch_crash_save_vmcoreinfo() into two new
arch/x86/kernel/crash_core_*.c files, which are gated behind
CONFIG_CRASH_CORE.

Signed-off-by: Omar Sandoval <[email protected]>
---
Based on Linus's tree.

arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/crash_core_32.c | 17 +++++++++++++++++
arch/x86/kernel/crash_core_64.c | 24 ++++++++++++++++++++++++
arch/x86/kernel/machine_kexec_32.c | 12 ------------
arch/x86/kernel/machine_kexec_64.c | 19 -------------------
5 files changed, 42 insertions(+), 31 deletions(-)
create mode 100644 arch/x86/kernel/crash_core_32.c
create mode 100644 arch/x86/kernel/crash_core_64.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 6175e370ee4a..9b294c13809a 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_FUNCTION_TRACER) += ftrace_$(BITS).o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_X86_TSC) += trace_clock.o
+obj-$(CONFIG_CRASH_CORE) += crash_core_$(BITS).o
obj-$(CONFIG_KEXEC_CORE) += machine_kexec_$(BITS).o
obj-$(CONFIG_KEXEC_CORE) += relocate_kernel_$(BITS).o crash.o
obj-$(CONFIG_KEXEC_FILE) += kexec-bzimage64.o
diff --git a/arch/x86/kernel/crash_core_32.c b/arch/x86/kernel/crash_core_32.c
new file mode 100644
index 000000000000..c0159a7bca6d
--- /dev/null
+++ b/arch/x86/kernel/crash_core_32.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/crash_core.h>
+
+#include <asm/pgtable.h>
+#include <asm/setup.h>
+
+void arch_crash_save_vmcoreinfo(void)
+{
+#ifdef CONFIG_NUMA
+ VMCOREINFO_SYMBOL(node_data);
+ VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
+#endif
+#ifdef CONFIG_X86_PAE
+ VMCOREINFO_CONFIG(X86_PAE);
+#endif
+}
diff --git a/arch/x86/kernel/crash_core_64.c b/arch/x86/kernel/crash_core_64.c
new file mode 100644
index 000000000000..845a57eb4eb7
--- /dev/null
+++ b/arch/x86/kernel/crash_core_64.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/crash_core.h>
+
+#include <asm/pgtable.h>
+#include <asm/setup.h>
+
+void arch_crash_save_vmcoreinfo(void)
+{
+ u64 sme_mask = sme_me_mask;
+
+ VMCOREINFO_NUMBER(phys_base);
+ VMCOREINFO_SYMBOL(init_top_pgt);
+ vmcoreinfo_append_str("NUMBER(pgtable_l5_enabled)=%d\n",
+ pgtable_l5_enabled());
+
+#ifdef CONFIG_NUMA
+ VMCOREINFO_SYMBOL(node_data);
+ VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
+#endif
+ vmcoreinfo_append_str("KERNELOFFSET=%lx\n", kaslr_offset());
+ VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
+ VMCOREINFO_NUMBER(sme_mask);
+}
diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
index 7b45e8daad22..02bddfc122a4 100644
--- a/arch/x86/kernel/machine_kexec_32.c
+++ b/arch/x86/kernel/machine_kexec_32.c
@@ -250,15 +250,3 @@ void machine_kexec(struct kimage *image)

__ftrace_enabled_restore(save_ftrace_enabled);
}
-
-void arch_crash_save_vmcoreinfo(void)
-{
-#ifdef CONFIG_NUMA
- VMCOREINFO_SYMBOL(node_data);
- VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
-#endif
-#ifdef CONFIG_X86_PAE
- VMCOREINFO_CONFIG(X86_PAE);
-#endif
-}
-
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 16e125a50b33..ad5cdd6a5f23 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -398,25 +398,6 @@ void machine_kexec(struct kimage *image)
__ftrace_enabled_restore(save_ftrace_enabled);
}

-void arch_crash_save_vmcoreinfo(void)
-{
- u64 sme_mask = sme_me_mask;
-
- VMCOREINFO_NUMBER(phys_base);
- VMCOREINFO_SYMBOL(init_top_pgt);
- vmcoreinfo_append_str("NUMBER(pgtable_l5_enabled)=%d\n",
- pgtable_l5_enabled());
-
-#ifdef CONFIG_NUMA
- VMCOREINFO_SYMBOL(node_data);
- VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
-#endif
- vmcoreinfo_append_str("KERNELOFFSET=%lx\n",
- kaslr_offset());
- VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
- VMCOREINFO_NUMBER(sme_mask);
-}
-
/* arch-dependent functionality related to kexec file-based syscall */

#ifdef CONFIG_KEXEC_FILE
--
2.24.0


2019-12-19 20:29:30

by Omar Sandoval

[permalink] [raw]
Subject: Re: [PATCH] x86: define arch_crash_save_vmcoreinfo() if CONFIG_CRASH_CORE=y

On Wed, Dec 18, 2019 at 11:41:13AM +0100, Borislav Petkov wrote:
> On Mon, Dec 02, 2019 at 10:02:29AM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <[email protected]>
> >
> > kernel/crash_core.c calls arch_crash_save_vmcoreinfo() to get
> > arch-specific bits for vmcoreinfo. If it is not defined, then it has a
> > no-op fallback. kernel/crash_core.c is gated behind CONFIG_CRASH_CORE.
> > However, x86 defines arch_crash_save_vmcoreinfo() in
> > arch/x86/kernel/machine_kexec_*.c, which is gated behind
> > CONFIG_KEXEC_CORE. So, a kernel with CONFIG_CRASH_CORE=y and
> > CONFIG_KEXEC_CORE=n
>
> How does that even happen?
>
> Symbol: KEXEC_CORE [=y]
> Type : bool
> Defined at arch/Kconfig:17
> Selects: CRASH_CORE [=y]
> Selected by [y]:
> - KEXEC [=y]
> - KEXEC_FILE [=y] && X86_64 [=y] && CRYPTO [=y]=y && CRYPTO_SHA256 [=y]=y
>
> In order to do crash dumps, you need to select KEXEC, which selects
> KEXEC_CORE, which selects CRASH_CORE...
>
> Or are you talking about the PROC_KCORE use angle where it selects
> CRASH_CORE and the crash_save_vmcoreinfo_init() initcall is then
> supposed to save arch-specific crash info?

Yes, I'm talking about reading VMCOREINFO from /proc/kcore at runtime,
no kdump involved. crash [1] and my own tool, drgn [2], use this for
live debugging.

I can set CONFIG_PROC_KCORE=y, which selects CONFIG_CRASH_CORE=y, but if
CONFIG_KEXEC_CORE=n, the VMCOREINFO is incomplete.

1: https://github.com/crash-utility/crash/commit/60a42d709280cdf38ab06327a5b4fa9d9208ef86
2: https://github.com/osandov/drgn/blob/73dd7def1217e24cc83d8ca95c995decbd9ba24c/libdrgn/program.c#L385

2019-12-20 10:08:11

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] x86: define arch_crash_save_vmcoreinfo() if CONFIG_CRASH_CORE=y

On Thu, Dec 19, 2019 at 12:28:07PM -0800, Omar Sandoval wrote:
> Yes, I'm talking about reading VMCOREINFO from /proc/kcore at runtime,
> no kdump involved. crash [1] and my own tool, drgn [2], use this for
> live debugging.

Ok, please state the gist of the use cases in the commit message so that
it is clear why you're doing this.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette