[PATCH 0/9] x86/kexec: Fixes for tip/x86/boot

[PATCH 0/9] x86/kexec: Fixes for tip/x86/boot

[David Woodhouse]

Fix a few bugs introduced by the recent rework of the relocate_kernel()
code, and one which has been there in the kjump code from the beginning.

 • Global read-only TLB entries made relocate_kernel() crash when writing
   to its own page after the %cr3 switch should have made it writeable.
 • The preserve_context flag was lost when invoking swap_pages on the way
   back, causing pages not to be swapped.
 • The wrong page was being used as the swap page.
 • The ABI for the kjump call asks the callee to put its entry address for
   next time at %rsp+8 before returning, but we set %rsp to the top of the
   available page, such that the entry address is at the start of some
   other page.
 • The relocate_kernel() function lacked Clang CFI information, but is now
   called via a function pointer, leading to a crash¹.
 • The relocate_kernel() code and data could end up being linked into the
   wrong place for a LTO / -ffunction-sections build.

Thanks to Nathan for reporting many of the above.

Also a few minor cleanups, including a comments-only patch from Rafael 
on the suspend-like part of kjump as a prelude to actually cleaning that 
up.

¹ Fixed by just adding __nocfi. Actually providing the CFI information 
for relocate_kernel() will need a bit more work, so let's just do the 
simple fix for now.

David Woodhouse (7):
      x86/kexec: Disable global pages before writing to control page
      x86/kexec: Ensure preserve_context flag is set on return to kernel
      x86/kexec: Use correct swap page in swap_pages function
      x86/kexec: Fix stack and handling of re-entry point for ::preserve_context
      x86/kexec: Mark machine_kexec() with __nocfi
      x86/kexec: Cope with relocate_kernel() not being at the start of the page
      x86/kexec: Use typedef for relocate_kernel_fn function prototype

Nathan Chancellor (1):
      x86/kexec: Fix location of relocate_kernel with -ffunction-sections

Rafael J. Wysocki (1):
      kexec_core: Add and update comments regarding the KEXEC_JUMP flow

 arch/x86/include/asm/kexec.h         | 26 ++++++++--------
 arch/x86/kernel/machine_kexec_32.c   |  7 +----
 arch/x86/kernel/machine_kexec_64.c   |  8 ++---
 arch/x86/kernel/relocate_kernel_64.S | 57 ++++++++++++++++++++++++------------
 arch/x86/kernel/vmlinux.lds.S        |  4 +--
 kernel/kexec_core.c                  | 23 +++++++++++----
 6 files changed, 74 insertions(+), 51 deletions(-)

[PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The kernel switches to a new set of page tables during kexec. The global
mappings (_PAGE_GLOBAL==1) can remain in the TLB after this switch. This
is generally not a problem because the new page tables use a different
portion of the virtual address space than the normal kernel mappings.

The critical exception to that generalisation (and the only mapping
which isn't an identity mapping) is the kexec control page itself —
which was ROX in the original kernel mapping, but should be RWX in the
new page tables. If there is a global TLB entry for that in its prior
read-only state, it definitely needs to be flushed before attempting to
write through that virtual mapping.

It would be possible to just avoid writing to the virtual address of the
page and defer all writes until they can be done through the identity
mapping. But there's no good reason to keep the old TLB entries around,
as they can cause nothing but trouble.

Clear the PGE bit in %cr4 early, before storing data in the control page.

Fixes: 5a82223e0743 ("x86/kexec: Mark relocate_kernel page as ROX instead of RWX")
Co-authored-by: Dave Hansen <dave.hansen@linux.intel.com>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: "Ning, Hongyu" <hongyu.ning@linux.intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219592
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: "Ning, Hongyu" <hongyu.ning@linux.intel.com>
---
 arch/x86/kernel/relocate_kernel_64.S | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 8bc86a1e056a..9bd601dd8659 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -70,14 +70,20 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
 	movq	kexec_pa_table_page(%rip), %r9
 	movq	%r9, %cr3
 
+	/* Leave CR4 in %r13 to enable the right paging mode later. */
+	movq	%cr4, %r13
+
+	/* Disable global pages immediately to ensure this mapping is RWX */
+	movq	%r13, %r12
+	andq	$~(X86_CR4_PGE), %r12
+	movq	%r12, %cr4
+
 	/* Save %rsp and CRs. */
+	movq	%r13, saved_cr4(%rip)
 	movq    %rsp, saved_rsp(%rip)
 	movq	%rax, saved_cr3(%rip)
 	movq	%cr0, %rax
 	movq	%rax, saved_cr0(%rip)
-	/* Leave CR4 in %r13 to enable the right paging mode later. */
-	movq	%cr4, %r13
-	movq	%r13, saved_cr4(%rip)
 
 	/* save indirection list for jumping back */
 	movq	%rdi, pa_backup_pages_map(%rip)

base-commit: 35aafa1d41cee0d3d50164561bca34befc1d9ce3
-- 
2.47.0

[PATCH 2/9] x86/kexec: Ensure preserve_context flag is set on return to kernel

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The swap_pages function will only actually *swap*, as its name implies,
if the preserve_context flag in the %r11 register is non-zero. On the
way back from a ::preserve_context kexec, ensure that the %r11 register
is non-zero so that the pages get swapped back.

Fixes: 9e5683e2d0b5 ("x86/kexec: Only swap pages for ::preserve_context mode")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 9bd601dd8659..1a52e4339c1d 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -220,6 +220,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	movq	kexec_pa_table_page(%rip), %rax
 	movq	%rax, %cr3
 	lea	PAGE_SIZE(%r8), %rsp
+	movq	$1, %r11	/* Ensure preserve_context flag is set */
 	call	swap_pages
 	movq	kexec_va_control_page(%rip), %rax
 	addq	$(virtual_mapped - relocate_kernel), %rax
-- 
2.47.0

[PATCH 3/9] x86/kexec: Use correct swap page in swap_pages function

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The swap_pages function expects the swap page to be in %r10, but there
was no documentation to that effect. Once upon a time the setup code
used to load its value from a kernel virtual address and save it to an
address which is accessible in the identity-mapped page tables, and
*happened* to use %r10 to do so, with no comment that it was left there
on *purpose* instead of just being a scratch register. Once that was no
longer necessary, %r10 just holds whatever the kernel happened to leave
in it.

Now that the original value passed by the kernel is accessible via
%rip-relative addressing, load directly from there instead of using %r10
for it. But document the other parameters that the swap_pages function
*does* expect in registers.

Fixes: b3adabae8a96 ("x86/kexec: Drop page_list argument from relocate_kernel()")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 1a52e4339c1d..0d6fce1e0a32 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -264,6 +264,10 @@ SYM_CODE_END(virtual_mapped)
 	/* Do the copies */
 SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
 	UNWIND_HINT_END_OF_STACK
+	/*
+	 * %rdi indirection page
+	 * %r11 preserve_context
+	 */
 	movq	%rdi, %rcx	/* Put the indirection_page in %rcx */
 	xorl	%edi, %edi
 	xorl	%esi, %esi
@@ -302,7 +306,7 @@ SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
 	jz	.Lnoswap
 
 	/* copy source page to swap page */
-	movq	%r10, %rdi
+	movq	kexec_pa_swap_page(%rip), %rdi
 	movl	$512, %ecx
 	rep ; movsq
 
@@ -314,7 +318,7 @@ SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
 
 	/* copy swap page to destination page */
 	movq	%rdx, %rdi
-	movq	%r10, %rsi
+	movq	kexec_pa_swap_page(%rip), %rsi
 .Lnoswap:
 	movl	$512, %ecx
 	rep ; movsq
-- 
2.47.0

[PATCH 4/9] x86/kexec: Fix stack and handling of re-entry point for ::preserve_context

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

A ::preserve_context kimage can be invoked more than once, and the entry
point can be different every time. When the callee returns to the kernel,
it leaves the address of its entry point for next time on the stack.

That being the case, one might reasonably assume that the caller would
allocate space for it on the stack fram before actually performing the
'call' into the callee.

Apparently not, though. Ever since the kjump code was first added in
2009, it has set up a *new* stack at the top of the swap_page scratch
page, then just performed the 'call' without allocating any space for
the re-entry address to be returned. It then reads the re-entry point
for next time from 0(%rsp) which is actually the first qword of the page
*after* the swap page, which might not exist at all! And if the callee
has written to that, then it will have corrupted memory it doesn't own.

Correct this by pushing the entry point of the callee onto the stack
before calling it. The callee may then adjust it, or not, as it sees fit,
and subsequent invocations should work correctly either way.

Remove a stray push of zero to the *relocate_kernel* stack, which may
have been intended for this purpose, but which was actually just noise.

Also, loading the stack for the callee relied on the address of the swap
page being in %r10 without ever documenting that fact. Recent code
changes made that no longer true, so load it directly from the local
kexec_pa_swap_page variable instead.

Fixes: b3adabae8a96 ("x86/kexec: Drop page_list argument from relocate_kernel()")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 0d6fce1e0a32..b680f24896b8 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -113,8 +113,6 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 * %r13 original CR4 when relocate_kernel() was invoked
 	 */
 
-	/* set return address to 0 if not preserving context */
-	pushq	$0
 	/* store the start address on the stack */
 	pushq   %rdx
 
@@ -208,12 +206,19 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 
 .Lrelocate:
 	popq	%rdx
+
+	/* Use the swap page for the callee's stack */
+	movq	kexec_pa_swap_page(%rip), %r10
 	leaq	PAGE_SIZE(%r10), %rsp
+
+	/* push the existing entry point onto the callee's stack */
+	pushq	%rdx
+
 	ANNOTATE_RETPOLINE_SAFE
 	call	*%rdx
 
 	/* get the re-entry point of the peer system */
-	movq	0(%rsp), %rbp
+	popq	%rbp
 	leaq	relocate_kernel(%rip), %r8
 	movq	kexec_pa_swap_page(%rip), %r10
 	movq	pa_backup_pages_map(%rip), %rdi
@@ -247,6 +252,7 @@ SYM_CODE_START_LOCAL_NOALIGN(virtual_mapped)
 	lgdt    saved_context_gdt_desc(%rax)
 #endif
 
+	/* relocate_kernel() returns the re-entry point for next time */
 	movq	%rbp, %rax
 
 	popf
-- 
2.47.0

[PATCH 5/9] x86/kexec: Fix location of relocate_kernel with -ffunction-sections

[David Woodhouse]

From: Nathan Chancellor <nathan@kernel.org>

After commit cb33ff9e063c ("x86/kexec: Move relocate_kernel to kernel
.data section"), kernels configured with an option that uses
-ffunction-sections, such as CONFIG_LTO_CLANG, crash when kexecing
because the value of relocate_kernel does not match the value of
__relocate_kernel_start so incorrect code gets copied via
machine_kexec_prepare().

  $ llvm-nm good-vmlinux &| rg relocate_kernel
  ffffffff83280d41 T __relocate_kernel_end
  ffffffff83280b00 T __relocate_kernel_start
  ffffffff83280b00 T relocate_kernel

  $ llvm-nm bad-vmlinux &| rg relocate_kernel
  ffffffff83266100 D __relocate_kernel_end
  ffffffff83266100 D __relocate_kernel_start
  ffffffff8120b0d8 T relocate_kernel

When -ffunction-sections is enabled, TEXT_MAIN matches on
'.text.[0-9a-zA-Z_]*' to coalesce the function specific functions back
into .text during link time after they have been optimized. Due to the
placement of TEXT_TEXT before KEXEC_RELOCATE_KERNEL in the x86 linker
script, the .text.relocate_kernel section ends up in .text instead of
.data.

Use a second dot in the relocate_kernel section name to avoid matching
on TEXT_MAIN, which matches a similar situation that happened in
commit 79cd2a11224e ("x86/retpoline,kprobes: Fix position of thunk
sections with CONFIG_LTO_CLANG"), which allows kexec to function
properly.

While .data.relocate_kernel still ends up in the .data section via
DATA_MAIN -> DATA_DATA, ensure it is located with the
.text.relocate_kernel section as intended by performing the same
transformation.

Fixes: cb33ff9e063c ("x86/kexec: Move relocate_kernel to kernel .data section")
Fixes: 8dbec5c77bc3 ("x86/kexec: Add data section to relocate_kernel")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 6 +++---
 arch/x86/kernel/vmlinux.lds.S        | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index b680f24896b8..1996cea909ff 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -23,11 +23,11 @@
 #define PAGE_ATTR (_PAGE_PRESENT | _PAGE_RW | _PAGE_ACCESSED | _PAGE_DIRTY)
 
 /*
- * The .text.relocate_kernel and .data.relocate_kernel sections are copied
+ * The .text..relocate_kernel and .data..relocate_kernel sections are copied
  * into the control page, and the remainder of the page is used as the stack.
  */
 
-	.section .data.relocate_kernel,"a";
+	.section .data..relocate_kernel,"a";
 /* Minimal CPU state */
 SYM_DATA_LOCAL(saved_rsp, .quad 0)
 SYM_DATA_LOCAL(saved_cr0, .quad 0)
@@ -39,7 +39,7 @@ SYM_DATA(kexec_pa_table_page, .quad 0)
 SYM_DATA(kexec_pa_swap_page, .quad 0)
 SYM_DATA_LOCAL(pa_backup_pages_map, .quad 0)
 
-	.section .text.relocate_kernel,"ax";
+	.section .text..relocate_kernel,"ax";
 	.code64
 SYM_CODE_START_NOALIGN(relocate_kernel)
 	UNWIND_HINT_END_OF_STACK
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 0c893997f023..63ff60a11be5 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -100,8 +100,8 @@ const_pcpu_hot = pcpu_hot;
 #define KEXEC_RELOCATE_KERNEL					\
 	. = ALIGN(0x100);					\
 	__relocate_kernel_start = .;				\
-	*(.text.relocate_kernel);				\
-	*(.data.relocate_kernel);				\
+	*(.text..relocate_kernel);				\
+	*(.data..relocate_kernel);				\
 	__relocate_kernel_end = .;
 
 ASSERT(__relocate_kernel_end - __relocate_kernel_start <= KEXEC_CONTROL_CODE_MAX_SIZE,
-- 
2.47.0

[PATCH 6/9] x86/kexec: Mark machine_kexec() with __nocfi

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

A recent commit caused the relocate_kernel() function to be invoked
through a function pointer, but it does not have CFI information. The
resulting trap occurs after the IDT and GDT have been invalidated,
leading to a triple-fault if CONFIG_CFI_CLANG is enabled.

Using SYM_TYPED_FUNC_START() to provide the CFI information looks like
it will require a prolonged battle with objtool. And is fairly pointless
anyway, as the actual signature comes from a __kcfi_typeid_… symbol
emitted from the C code based on the function prototype it thinks that
relocate_kernel has, rendering the check somewhat tautological.

The simple fix is just to mark machine_kexec() with __nocfi.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Fixes: eeebbde57113 ("x86/kexec: Invoke copy of relocate_kernel() instead of
the original")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/machine_kexec_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 9232ad1562c8..1440f792a86d 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -342,7 +342,7 @@ void machine_kexec_cleanup(struct kimage *image)
  * Do not allocate memory (or fail in any way) in machine_kexec().
  * We are past the point of no return, committed to rebooting now.
  */
-void machine_kexec(struct kimage *image)
+void __nocfi machine_kexec(struct kimage *image)
 {
 	unsigned long (*relocate_kernel_ptr)(unsigned long indirection_page,
 					     unsigned long pa_control_page,
-- 
2.47.0

[PATCH 7/9] kexec_core: Add and update comments regarding the KEXEC_JUMP flow

[David Woodhouse]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

The KEXEC_JUMP flow is analogous to hibernation flows occurring before
and after creating an image and before and after jumping from the
restore kernel to the image one, which is why it uses the same device
callbacks as those hibernation flows.

Add comments explaining that to the code in question and update an
existing comment in it which appears a bit out of context.

No functional changes.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 kernel/kexec_core.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index c0caa14880c3..7cf8437e0f38 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1001,6 +1001,12 @@ int kernel_kexec(void)
 
 #ifdef CONFIG_KEXEC_JUMP
 	if (kexec_image->preserve_context) {
+		/*
+		 * This flow is analogous to hibernation flows that occur before
+		 * creating an image and before jumping from the restore kernel
+		 * to the image one, so it uses the same device device callbacks
+		 * as those two flows.
+		 */
 		pm_prepare_console();
 		error = freeze_processes();
 		if (error) {
@@ -1011,12 +1017,10 @@ int kernel_kexec(void)
 		error = dpm_suspend_start(PMSG_FREEZE);
 		if (error)
 			goto Resume_console;
-		/* At this point, dpm_suspend_start() has been called,
-		 * but *not* dpm_suspend_end(). We *must* call
-		 * dpm_suspend_end() now.  Otherwise, drivers for
-		 * some devices (e.g. interrupt controllers) become
-		 * desynchronized with the actual state of the
-		 * hardware at resume time, and evil weirdness ensues.
+		/*
+		 * dpm_suspend_end() must be called after dpm_suspend_start()
+		 * to complete the transition, like in the hibernation flows
+		 * mentioned above.
 		 */
 		error = dpm_suspend_end(PMSG_FREEZE);
 		if (error)
@@ -1052,6 +1056,13 @@ int kernel_kexec(void)
 
 #ifdef CONFIG_KEXEC_JUMP
 	if (kexec_image->preserve_context) {
+		/*
+		 * This flow is analogous to hibernation flows that occur after
+		 * creating an image and after the image hernel has got control
+		 * back, and in case the devices have been reset or otherwise
+		 * manipulated in the meantime, it uses the device callbacks
+		 * used by the latter.
+		 */
 		syscore_resume();
  Enable_irqs:
 		local_irq_enable();
-- 
2.47.0

[PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

A few places in the kexec control code page make the assumption that the
first instruction of relocate_kernel is at the very start of the page.

To allow for Clang CFI information to be added to relocate_kernel(), as
well as the general principle of removing unwarranted assumptions, fix
them to use the external __relocate_kernel_start symbol that the linker
adds. This means using a separate addq and subq for calculating offsets,
as the assembler can no longer calculate the delta directly for itself
and relocations aren't that versatile.

Turn the jump from relocate_kernel() to identity_mapped() into a real
indirect 'jmp *%rsi' too, while touching it. There was no real reason
for it to be a push+ret in the first place, and adding Clang CFI info
will also give objtool enough visibility to start complaining 'return
with modified stack frame' about it.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 1996cea909ff..d74798d78263 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -95,11 +95,10 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
 	lea	PAGE_SIZE(%rsi), %rsp
 
 	/* jump to identity mapped page */
-	addq	$(identity_mapped - relocate_kernel), %rsi
-	pushq	%rsi
-	ANNOTATE_UNRET_SAFE
-	ret
-	int3
+	addq	$identity_mapped, %rsi
+	subq	$__relocate_kernel_start, %rsi
+	ANNOTATE_RETPOLINE_SAFE
+	jmp	*%rsi
 SYM_CODE_END(relocate_kernel)
 
 SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
@@ -219,16 +218,21 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 
 	/* get the re-entry point of the peer system */
 	popq	%rbp
-	leaq	relocate_kernel(%rip), %r8
 	movq	kexec_pa_swap_page(%rip), %r10
 	movq	pa_backup_pages_map(%rip), %rdi
 	movq	kexec_pa_table_page(%rip), %rax
 	movq	%rax, %cr3
+
+	/* Find start (and end) of this physical mapping of control page */
+	leaq	(%rip), %r8
+	ANNOTATE_NOENDBR
+	andq	$PAGE_MASK, %r8
 	lea	PAGE_SIZE(%r8), %rsp
 	movq	$1, %r11	/* Ensure preserve_context flag is set */
 	call	swap_pages
 	movq	kexec_va_control_page(%rip), %rax
-	addq	$(virtual_mapped - relocate_kernel), %rax
+	addq	$virtual_mapped, %rax
+	subq	$__relocate_kernel_start, %rax
 	pushq	%rax
 	ANNOTATE_UNRET_SAFE
 	ret
-- 
2.47.0

[PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Both i386 and x86_64 now copy the relocate_kernel function into the control
page and execute it from there, using an open-coded function pointer.

Use a typedef for it instead.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
 arch/x86/kernel/machine_kexec_32.c |  7 +------
 arch/x86/kernel/machine_kexec_64.c |  6 +-----
 3 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index 48e4f44f794f..8ad187462b68 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
 }
 
 #ifdef CONFIG_X86_32
-asmlinkage unsigned long
-relocate_kernel(unsigned long indirection_page,
-		unsigned long control_page,
-		unsigned long start_address,
-		unsigned int has_pae,
-		unsigned int preserve_context);
+typedef asmlinkage unsigned long
+relocate_kernel_fn(unsigned long indirection_page,
+		   unsigned long control_page,
+		   unsigned long start_address,
+		   unsigned int has_pae,
+		   unsigned int preserve_context);
 #else
-unsigned long
-relocate_kernel(unsigned long indirection_page,
-		unsigned long pa_control_page,
-		unsigned long start_address,
-		unsigned int preserve_context,
-		unsigned int host_mem_enc_active);
+typedef unsigned long
+relocate_kernel_fn(unsigned long indirection_page,
+		   unsigned long pa_control_page,
+		   unsigned long start_address,
+		   unsigned int preserve_context,
+		   unsigned int host_mem_enc_active);
 #endif
-
+extern relocate_kernel_fn relocate_kernel;
 #define ARCH_HAS_KIMAGE_ARCH
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
index 1b373d79cedc..80265162aeff 100644
--- a/arch/x86/kernel/machine_kexec_32.c
+++ b/arch/x86/kernel/machine_kexec_32.c
@@ -160,15 +160,10 @@ void machine_kexec_cleanup(struct kimage *image)
  */
 void machine_kexec(struct kimage *image)
 {
+	relocate_kernel_fn *relocate_kernel_ptr;
 	unsigned long page_list[PAGES_NR];
 	void *control_page;
 	int save_ftrace_enabled;
-	asmlinkage unsigned long
-		(*relocate_kernel_ptr)(unsigned long indirection_page,
-				       unsigned long control_page,
-				       unsigned long start_address,
-				       unsigned int has_pae,
-				       unsigned int preserve_context);
 
 #ifdef CONFIG_KEXEC_JUMP
 	if (image->preserve_context)
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 1440f792a86d..dd75a51463a2 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -344,12 +344,8 @@ void machine_kexec_cleanup(struct kimage *image)
  */
 void __nocfi machine_kexec(struct kimage *image)
 {
-	unsigned long (*relocate_kernel_ptr)(unsigned long indirection_page,
-					     unsigned long pa_control_page,
-					     unsigned long start_address,
-					     unsigned int preserve_context,
-					     unsigned int host_mem_enc_active);
 	unsigned long reloc_start = (unsigned long)__relocate_kernel_start;
+	relocate_kernel_fn *relocate_kernel_ptr;
 	unsigned int host_mem_enc_active;
 	int save_ftrace_enabled;
 	void *control_page;
-- 
2.47.0

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> A few places in the kexec control code page make the assumption that the
> first instruction of relocate_kernel is at the very start of the page.
>
> To allow for Clang CFI information to be added to relocate_kernel(), as
> well as the general principle of removing unwarranted assumptions, fix
> them to use the external __relocate_kernel_start symbol that the linker
> adds. This means using a separate addq and subq for calculating offsets,
> as the assembler can no longer calculate the delta directly for itself
> and relocations aren't that versatile.
>

You can still avoid the absolute relocations though, ...

> Turn the jump from relocate_kernel() to identity_mapped() into a real
> indirect 'jmp *%rsi' too, while touching it. There was no real reason
> for it to be a push+ret in the first place, and adding Clang CFI info
> will also give objtool enough visibility to start complaining 'return
> with modified stack frame' about it.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 1996cea909ff..d74798d78263 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -95,11 +95,10 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
>         lea     PAGE_SIZE(%rsi), %rsp
>
>         /* jump to identity mapped page */
> -       addq    $(identity_mapped - relocate_kernel), %rsi
> -       pushq   %rsi
> -       ANNOTATE_UNRET_SAFE
> -       ret
> -       int3
> +       addq    $identity_mapped, %rsi
> +       subq    $__relocate_kernel_start, %rsi

... if you turn this into

0:     addq    $identity_mapped - 0b, %rsi
       subq    $__relocate_kernel_start - 0b, %rsi


> +       ANNOTATE_RETPOLINE_SAFE
> +       jmp     *%rsi
>  SYM_CODE_END(relocate_kernel)
>
>  SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> @@ -219,16 +218,21 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>
>         /* get the re-entry point of the peer system */
>         popq    %rbp
> -       leaq    relocate_kernel(%rip), %r8
>         movq    kexec_pa_swap_page(%rip), %r10
>         movq    pa_backup_pages_map(%rip), %rdi
>         movq    kexec_pa_table_page(%rip), %rax
>         movq    %rax, %cr3
> +
> +       /* Find start (and end) of this physical mapping of control page */
> +       leaq    (%rip), %r8
> +       ANNOTATE_NOENDBR
> +       andq    $PAGE_MASK, %r8
>         lea     PAGE_SIZE(%r8), %rsp
>         movq    $1, %r11        /* Ensure preserve_context flag is set */
>         call    swap_pages
>         movq    kexec_va_control_page(%rip), %rax
> -       addq    $(virtual_mapped - relocate_kernel), %rax
> +       addq    $virtual_mapped, %rax
> +       subq    $__relocate_kernel_start, %rax
>         pushq   %rax
>         ANNOTATE_UNRET_SAFE
>         ret
> --
> 2.47.0
>

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Both i386 and x86_64 now copy the relocate_kernel function into the control
> page and execute it from there, using an open-coded function pointer.
>
> Use a typedef for it instead.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
>  arch/x86/kernel/machine_kexec_32.c |  7 +------
>  arch/x86/kernel/machine_kexec_64.c |  6 +-----
>  3 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 48e4f44f794f..8ad187462b68 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
>  }
>
>  #ifdef CONFIG_X86_32
> -asmlinkage unsigned long
> -relocate_kernel(unsigned long indirection_page,
> -               unsigned long control_page,
> -               unsigned long start_address,
> -               unsigned int has_pae,
> -               unsigned int preserve_context);
> +typedef asmlinkage unsigned long
> +relocate_kernel_fn(unsigned long indirection_page,
> +                  unsigned long control_page,
> +                  unsigned long start_address,
> +                  unsigned int has_pae,
> +                  unsigned int preserve_context);

linkage is not part of the type. 'asmlinkage' is #define'd to the
empty string today, so it doesn't matter, but better to omit it here.

>  #else
> -unsigned long
> -relocate_kernel(unsigned long indirection_page,
> -               unsigned long pa_control_page,
> -               unsigned long start_address,
> -               unsigned int preserve_context,
> -               unsigned int host_mem_enc_active);
> +typedef unsigned long
> +relocate_kernel_fn(unsigned long indirection_page,
> +                  unsigned long pa_control_page,
> +                  unsigned long start_address,
> +                  unsigned int preserve_context,
> +                  unsigned int host_mem_enc_active);
>  #endif
> -
> +extern relocate_kernel_fn relocate_kernel;
>  #define ARCH_HAS_KIMAGE_ARCH
>
>  #ifdef CONFIG_X86_32
> diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
> index 1b373d79cedc..80265162aeff 100644
> --- a/arch/x86/kernel/machine_kexec_32.c
> +++ b/arch/x86/kernel/machine_kexec_32.c
> @@ -160,15 +160,10 @@ void machine_kexec_cleanup(struct kimage *image)
>   */
>  void machine_kexec(struct kimage *image)
>  {
> +       relocate_kernel_fn *relocate_kernel_ptr;
>         unsigned long page_list[PAGES_NR];
>         void *control_page;
>         int save_ftrace_enabled;
> -       asmlinkage unsigned long
> -               (*relocate_kernel_ptr)(unsigned long indirection_page,
> -                                      unsigned long control_page,
> -                                      unsigned long start_address,
> -                                      unsigned int has_pae,
> -                                      unsigned int preserve_context);
>
>  #ifdef CONFIG_KEXEC_JUMP
>         if (image->preserve_context)
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 1440f792a86d..dd75a51463a2 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -344,12 +344,8 @@ void machine_kexec_cleanup(struct kimage *image)
>   */
>  void __nocfi machine_kexec(struct kimage *image)
>  {
> -       unsigned long (*relocate_kernel_ptr)(unsigned long indirection_page,
> -                                            unsigned long pa_control_page,
> -                                            unsigned long start_address,
> -                                            unsigned int preserve_context,
> -                                            unsigned int host_mem_enc_active);
>         unsigned long reloc_start = (unsigned long)__relocate_kernel_start;
> +       relocate_kernel_fn *relocate_kernel_ptr;
>         unsigned int host_mem_enc_active;
>         int save_ftrace_enabled;
>         void *control_page;
> --
> 2.47.0
>

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[David Woodhouse]

On 17 December 2024 09:47:36 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> A few places in the kexec control code page make the assumption that the
>> first instruction of relocate_kernel is at the very start of the page.
>>
>> To allow for Clang CFI information to be added to relocate_kernel(), as
>> well as the general principle of removing unwarranted assumptions, fix
>> them to use the external __relocate_kernel_start symbol that the linker
>> adds. This means using a separate addq and subq for calculating offsets,
>> as the assembler can no longer calculate the delta directly for itself
>> and relocations aren't that versatile.
>>
>
>You can still avoid the absolute relocations though, ...
...
>> +       addq    $identity_mapped, %rsi
>> +       subq    $__relocate_kernel_start, %rsi
>
>... if you turn this into
>
>0:     addq    $identity_mapped - 0b, %rsi
>       subq    $__relocate_kernel_start - 0b, %rsi

Is there any benefit to doing so? Are absolute relocations problematic?

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

On 17 December 2024 09:49:04 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> Both i386 and x86_64 now copy the relocate_kernel function into the control
>> page and execute it from there, using an open-coded function pointer.
>>
>> Use a typedef for it instead.
>>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>  arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
>>  arch/x86/kernel/machine_kexec_32.c |  7 +------
>>  arch/x86/kernel/machine_kexec_64.c |  6 +-----
>>  3 files changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
>> index 48e4f44f794f..8ad187462b68 100644
>> --- a/arch/x86/include/asm/kexec.h
>> +++ b/arch/x86/include/asm/kexec.h
>> @@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
>>  }
>>
>>  #ifdef CONFIG_X86_32
>> -asmlinkage unsigned long
>> -relocate_kernel(unsigned long indirection_page,
>> -               unsigned long control_page,
>> -               unsigned long start_address,
>> -               unsigned int has_pae,
>> -               unsigned int preserve_context);
>> +typedef asmlinkage unsigned long
>> +relocate_kernel_fn(unsigned long indirection_page,
>> +                  unsigned long control_page,
>> +                  unsigned long start_address,
>> +                  unsigned int has_pae,
>> +                  unsigned int preserve_context);
>
>linkage is not part of the type. 'asmlinkage' is #define'd to the
>empty string today, so it doesn't matter, but better to omit it here.

This is the i386 version. I thought ut was something like regparm(3) there?

And... WTF? How is the calling convention not part of the fundamental type of the function? If I have a pointer to such a function, using this typedef to ensure we all share the same prototype, are you telling me all the users of the typedef have to remember to tag that part on for themselves?

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 10:17, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On 17 December 2024 09:47:36 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
> >On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
> >>
> >> From: David Woodhouse <dwmw@amazon.co.uk>
> >>
> >> A few places in the kexec control code page make the assumption that the
> >> first instruction of relocate_kernel is at the very start of the page.
> >>
> >> To allow for Clang CFI information to be added to relocate_kernel(), as
> >> well as the general principle of removing unwarranted assumptions, fix
> >> them to use the external __relocate_kernel_start symbol that the linker
> >> adds. This means using a separate addq and subq for calculating offsets,
> >> as the assembler can no longer calculate the delta directly for itself
> >> and relocations aren't that versatile.
> >>
> >
> >You can still avoid the absolute relocations though, ...
> ...
> >> +       addq    $identity_mapped, %rsi
> >> +       subq    $__relocate_kernel_start, %rsi
> >
> >... if you turn this into
> >
> >0:     addq    $identity_mapped - 0b, %rsi
> >       subq    $__relocate_kernel_start - 0b, %rsi
>
> Is there any benefit to doing so? Are absolute relocations problematic?

Every absolute relocation produces an entry in the relocation table
that needs to be applied at every boot when KASLR is in effect. Beyond
that, it doesn't matter.

I've looked into PIC codegen/PIE linking for the core kernel, which is
why this caught my eye. If that effort ever advances, I'll need to
revisit this code as well and apply the change I suggested.

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 10:21, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On 17 December 2024 09:49:04 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
> >On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
> >>
> >> From: David Woodhouse <dwmw@amazon.co.uk>
> >>
> >> Both i386 and x86_64 now copy the relocate_kernel function into the control
> >> page and execute it from there, using an open-coded function pointer.
> >>
> >> Use a typedef for it instead.
> >>
> >> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> >> ---
> >>  arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
> >>  arch/x86/kernel/machine_kexec_32.c |  7 +------
> >>  arch/x86/kernel/machine_kexec_64.c |  6 +-----
> >>  3 files changed, 15 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> >> index 48e4f44f794f..8ad187462b68 100644
> >> --- a/arch/x86/include/asm/kexec.h
> >> +++ b/arch/x86/include/asm/kexec.h
> >> @@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
> >>  }
> >>
> >>  #ifdef CONFIG_X86_32
> >> -asmlinkage unsigned long
> >> -relocate_kernel(unsigned long indirection_page,
> >> -               unsigned long control_page,
> >> -               unsigned long start_address,
> >> -               unsigned int has_pae,
> >> -               unsigned int preserve_context);
> >> +typedef asmlinkage unsigned long
> >> +relocate_kernel_fn(unsigned long indirection_page,
> >> +                  unsigned long control_page,
> >> +                  unsigned long start_address,
> >> +                  unsigned int has_pae,
> >> +                  unsigned int preserve_context);
> >
> >linkage is not part of the type. 'asmlinkage' is #define'd to the
> >empty string today, so it doesn't matter, but better to omit it here.
>
> This is the i386 version. I thought ut was something like regparm(3) there?
>
> And... WTF? How is the calling convention not part of the fundamental type of the function? If I have a pointer to such a function, using this typedef to ensure we all share the same prototype, are you telling me all the users of the typedef have to remember to tag that part on for themselves?

No. I am talking about linkage not the calling convention.

Look at how __efiapi is used in the kernel if you would like to
understand the difference.

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[David Woodhouse]

On 17 December 2024 10:25:56 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 10:17, David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> On 17 December 2024 09:47:36 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>> >On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>> >>
>> >> From: David Woodhouse <dwmw@amazon.co.uk>
>> >>
>> >> A few places in the kexec control code page make the assumption that the
>> >> first instruction of relocate_kernel is at the very start of the page.
>> >>
>> >> To allow for Clang CFI information to be added to relocate_kernel(), as
>> >> well as the general principle of removing unwarranted assumptions, fix
>> >> them to use the external __relocate_kernel_start symbol that the linker
>> >> adds. This means using a separate addq and subq for calculating offsets,
>> >> as the assembler can no longer calculate the delta directly for itself
>> >> and relocations aren't that versatile.
>> >>
>> >
>> >You can still avoid the absolute relocations though, ...
>> ...
>> >> +       addq    $identity_mapped, %rsi
>> >> +       subq    $__relocate_kernel_start, %rsi
>> >
>> >... if you turn this into
>> >
>> >0:     addq    $identity_mapped - 0b, %rsi
>> >       subq    $__relocate_kernel_start - 0b, %rsi
>>
>> Is there any benefit to doing so? Are absolute relocations problematic?
>
>Every absolute relocation produces an entry in the relocation table
>that needs to be applied at every boot when KASLR is in effect. Beyond
>that, it doesn't matter.
>
>I've looked into PIC codegen/PIE linking for the core kernel, which is
>why this caught my eye. If that effort ever advances, I'll need to
>revisit this code as well and apply the change I suggested.

Ack, I'll roll it in to the next revision if Ingo hasn't rounded this set up of fixes up already. (On which topic, since I took the easy way out of the CFO thing, this patch isn't strictly needed to fix a regression in tip/x86/boot so can be dropped for now too.)

Thanks.

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

On 17 December 2024 10:29:21 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 10:21, David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> On 17 December 2024 09:49:04 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>> >On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
>> >>
>> >> From: David Woodhouse <dwmw@amazon.co.uk>
>> >>
>> >> Both i386 and x86_64 now copy the relocate_kernel function into the control
>> >> page and execute it from there, using an open-coded function pointer.
>> >>
>> >> Use a typedef for it instead.
>> >>
>> >> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> >> ---
>> >>  arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
>> >>  arch/x86/kernel/machine_kexec_32.c |  7 +------
>> >>  arch/x86/kernel/machine_kexec_64.c |  6 +-----
>> >>  3 files changed, 15 insertions(+), 24 deletions(-)
>> >>
>> >> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
>> >> index 48e4f44f794f..8ad187462b68 100644
>> >> --- a/arch/x86/include/asm/kexec.h
>> >> +++ b/arch/x86/include/asm/kexec.h
>> >> @@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
>> >>  }
>> >>
>> >>  #ifdef CONFIG_X86_32
>> >> -asmlinkage unsigned long
>> >> -relocate_kernel(unsigned long indirection_page,
>> >> -               unsigned long control_page,
>> >> -               unsigned long start_address,
>> >> -               unsigned int has_pae,
>> >> -               unsigned int preserve_context);
>> >> +typedef asmlinkage unsigned long
>> >> +relocate_kernel_fn(unsigned long indirection_page,
>> >> +                  unsigned long control_page,
>> >> +                  unsigned long start_address,
>> >> +                  unsigned int has_pae,
>> >> +                  unsigned int preserve_context);
>> >
>> >linkage is not part of the type. 'asmlinkage' is #define'd to the
>> >empty string today, so it doesn't matter, but better to omit it here.
>>
>> This is the i386 version. I thought ut was something like regparm(3) there?
>>
>> And... WTF? How is the calling convention not part of the fundamental type of the function? If I have a pointer to such a function, using this typedef to ensure we all share the same prototype, are you telling me all the users of the typedef have to remember to tag that part on for themselves?
>
>No. I am talking about linkage not the calling convention.

Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that 
<https://kernelnewbies.org/FAQ/asmlinkage> is a lie?

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 10:42, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On 17 December 2024 10:29:21 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
> >On Tue, 17 Dec 2024 at 10:21, David Woodhouse <dwmw2@infradead.org> wrote:
> >>
> >> On 17 December 2024 09:49:04 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
> >> >On Tue, 17 Dec 2024 at 00:37, David Woodhouse <dwmw2@infradead.org> wrote:
> >> >>
> >> >> From: David Woodhouse <dwmw@amazon.co.uk>
> >> >>
> >> >> Both i386 and x86_64 now copy the relocate_kernel function into the control
> >> >> page and execute it from there, using an open-coded function pointer.
> >> >>
> >> >> Use a typedef for it instead.
> >> >>
> >> >> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> >> >> ---
> >> >>  arch/x86/include/asm/kexec.h       | 26 +++++++++++++-------------
> >> >>  arch/x86/kernel/machine_kexec_32.c |  7 +------
> >> >>  arch/x86/kernel/machine_kexec_64.c |  6 +-----
> >> >>  3 files changed, 15 insertions(+), 24 deletions(-)
> >> >>
> >> >> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> >> >> index 48e4f44f794f..8ad187462b68 100644
> >> >> --- a/arch/x86/include/asm/kexec.h
> >> >> +++ b/arch/x86/include/asm/kexec.h
> >> >> @@ -111,21 +111,21 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
> >> >>  }
> >> >>
> >> >>  #ifdef CONFIG_X86_32
> >> >> -asmlinkage unsigned long
> >> >> -relocate_kernel(unsigned long indirection_page,
> >> >> -               unsigned long control_page,
> >> >> -               unsigned long start_address,
> >> >> -               unsigned int has_pae,
> >> >> -               unsigned int preserve_context);
> >> >> +typedef asmlinkage unsigned long
> >> >> +relocate_kernel_fn(unsigned long indirection_page,
> >> >> +                  unsigned long control_page,
> >> >> +                  unsigned long start_address,
> >> >> +                  unsigned int has_pae,
> >> >> +                  unsigned int preserve_context);
> >> >
> >> >linkage is not part of the type. 'asmlinkage' is #define'd to the
> >> >empty string today, so it doesn't matter, but better to omit it here.
> >>
> >> This is the i386 version. I thought ut was something like regparm(3) there?
> >>
> >> And... WTF? How is the calling convention not part of the fundamental type of the function? If I have a pointer to such a function, using this typedef to ensure we all share the same prototype, are you telling me all the users of the typedef have to remember to tag that part on for themselves?
> >
> >No. I am talking about linkage not the calling convention.
>
> Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that
> <https://kernelnewbies.org/FAQ/asmlinkage> is a lie?
>

It seems wildly out of date, at least.

Commit 96a388de5dc53a8b2 from 2007 removed the asmlinkage definition
containing regparm(0) from include/asm-i386/linkage.h, and I'm not
convinced it was ever sound to conflate linkage with calling
convention like that. Today, asmlinkage evaluates to 'extern "C"' when
using a C++ compiler, which is also not part of the type.

However, I failed to notice that this just moves code around, and only
applies to 32-bit in the first place. So I won't waste any more of
your time obsessing over this.

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

On 17 December 2024 10:54:19 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 10:42, David Woodhouse <dwmw2@infradead.org> wrote:
>> Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that
>> <https://kernelnewbies.org/FAQ/asmlinkage> is a lie?
>>
>
>It seems wildly out of date, at least.
>
>Commit 96a388de5dc53a8b2 from 2007 removed the asmlinkage definition
>containing regparm(0) from include/asm-i386/linkage.h, and I'm not
>convinced it was ever sound to conflate linkage with calling
>convention like that. Today, asmlinkage evaluates to 'extern "C"' when
>using a C++ compiler, which is also not part of the type.
>
>However, I failed to notice that this just moves code around, and only
>applies to 32-bit in the first place. So I won't waste any more of
>your time obsessing over this.

Too late :)

You've already made me concerned about what the calling convention *is* for relocate_kernel() on i386. Because if asmlinkage doesn't mean regparm(0) any more and the i386 kernel is still built with -mregparm=3, then how does the asm code (which seems to believe all its arguments are on the stack) actually work?

It seems slightly unlikely that kexec on i386 has just been broken since 2007 but I'm not sure I'd completely rule it out.

So now I guess I have to actually build a 32-bit userspace test case and *test* it.

And that means I no longer have any excuse for not doing all the same cleanups in the i386 version of the code that I've done for x86_64...

Thanks for that :)

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[Ard Biesheuvel]

On Tue, 17 Dec 2024 at 11:07, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On 17 December 2024 10:54:19 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
> >On Tue, 17 Dec 2024 at 10:42, David Woodhouse <dwmw2@infradead.org> wrote:
> >> Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that
> >> <https://kernelnewbies.org/FAQ/asmlinkage> is a lie?
> >>
> >
> >It seems wildly out of date, at least.
> >
> >Commit 96a388de5dc53a8b2 from 2007 removed the asmlinkage definition
> >containing regparm(0) from include/asm-i386/linkage.h, and I'm not
> >convinced it was ever sound to conflate linkage with calling
> >convention like that. Today, asmlinkage evaluates to 'extern "C"' when
> >using a C++ compiler, which is also not part of the type.
> >
> >However, I failed to notice that this just moves code around, and only
> >applies to 32-bit in the first place. So I won't waste any more of
> >your time obsessing over this.
>
> Too late :)
>
> You've already made me concerned about what the calling convention *is* for relocate_kernel() on i386. Because if asmlinkage doesn't mean regparm(0) any more and the i386 kernel is still built with -mregparm=3, then how does the asm code (which seems to believe all its arguments are on the stack) actually work?
>
> It seems slightly unlikely that kexec on i386 has just been broken since 2007 but I'm not sure I'd completely rule it out.
>
> So now I guess I have to actually build a 32-bit userspace test case and *test* it.
>
> And that means I no longer have any excuse for not doing all the same cleanups in the i386 version of the code that I've done for x86_64...
>
> Thanks for that :)
>

Actually, asmlinkage still means regparm(0) on i386, so I'm going to
have to apologise again, for my poor git foo this time.

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

On 17 December 2024 10:54:19 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 10:42, David Woodhouse <dwmw2@infradead.org> wrote:
>> Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that
>> <https://kernelnewbies.org/FAQ/asmlinkage> is a lie?
>>
>
>It seems wildly out of date, at least.
>
>Commit 96a388de5dc53a8b2 from 2007 removed the asmlinkage definition
>containing regparm(0) from include/asm-i386/linkage.h, 

No, it's still there in arch/x86/include/asm/linkage.h:
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/linkage.h#n20>

And maybe you're right that it's a poorly named macro and we shouldn't conflate calling convention with linkage. But I think it *should* be part of the typedef.

Re: [PATCH 9/9] x86/kexec: Use typedef for relocate_kernel_fn function prototype

[David Woodhouse]

On 17 December 2024 11:14:29 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Tue, 17 Dec 2024 at 11:07, David Woodhouse <dwmw2@infradead.org> wrote:
>>
>> On 17 December 2024 10:54:19 CET, Ard Biesheuvel <ardb@kernel.org> wrote:
>> >On Tue, 17 Dec 2024 at 10:42, David Woodhouse <dwmw2@infradead.org> wrote:
>> >> Hm, I am perfectly happy to believe that my memory is failing me, especially when it comes to specifics of i386 assembler code. But are you also telling me that
>> >> <https://kernelnewbies.org/FAQ/asmlinkage> is a lie?
>> >>
>> >
>> >It seems wildly out of date, at least.
>> >
>> >Commit 96a388de5dc53a8b2 from 2007 removed the asmlinkage definition
>> >containing regparm(0) from include/asm-i386/linkage.h, and I'm not
>> >convinced it was ever sound to conflate linkage with calling
>> >convention like that. Today, asmlinkage evaluates to 'extern "C"' when
>> >using a C++ compiler, which is also not part of the type.
>> >
>> >However, I failed to notice that this just moves code around, and only
>> >applies to 32-bit in the first place. So I won't waste any more of
>> >your time obsessing over this.
>>
>> Too late :)
>>
>> You've already made me concerned about what the calling convention *is* for relocate_kernel() on i386. Because if asmlinkage doesn't mean regparm(0) any more and the i386 kernel is still built with -mregparm=3, then how does the asm code (which seems to believe all its arguments are on the stack) actually work?
>>
>> It seems slightly unlikely that kexec on i386 has just been broken since 2007 but I'm not sure I'd completely rule it out.
>>
>> So now I guess I have to actually build a 32-bit userspace test case and *test* it.
>>
>> And that means I no longer have any excuse for not doing all the same cleanups in the i386 version of the code that I've done for x86_64...
>>
>> Thanks for that :)
>>
>
>Actually, asmlinkage still means regparm(0) on i386, so I'm going to
>have to apologise again, for my poor git foo this time.

Heh, no problem.

I'm pleased to find my memory wasn't failing me after all, so I might have a bit more time left before they put me out to pasture.

I'll take that win :)

Re: [PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[Kirill A. Shutemov]

On Mon, Dec 16, 2024 at 11:24:08PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The kernel switches to a new set of page tables during kexec. The global
> mappings (_PAGE_GLOBAL==1) can remain in the TLB after this switch. This
> is generally not a problem because the new page tables use a different
> portion of the virtual address space than the normal kernel mappings.
> 
> The critical exception to that generalisation (and the only mapping
> which isn't an identity mapping) is the kexec control page itself —
> which was ROX in the original kernel mapping, but should be RWX in the
> new page tables. If there is a global TLB entry for that in its prior
> read-only state, it definitely needs to be flushed before attempting to
> write through that virtual mapping.
> 
> It would be possible to just avoid writing to the virtual address of the
> page and defer all writes until they can be done through the identity
> mapping. But there's no good reason to keep the old TLB entries around,
> as they can cause nothing but trouble.
> 
> Clear the PGE bit in %cr4 early, before storing data in the control page.

It worth noting that flipping CR4.PGE triggers TLB flush. I was not sure
if CR3 write is required to make it happen.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[David Woodhouse]

On 17 December 2024 13:25:48 CET, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
>On Mon, Dec 16, 2024 at 11:24:08PM +0000, David Woodhouse wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>> 
>> The kernel switches to a new set of page tables during kexec. The global
>> mappings (_PAGE_GLOBAL==1) can remain in the TLB after this switch. This
>> is generally not a problem because the new page tables use a different
>> portion of the virtual address space than the normal kernel mappings.
>> 
>> The critical exception to that generalisation (and the only mapping
>> which isn't an identity mapping) is the kexec control page itself —
>> which was ROX in the original kernel mapping, but should be RWX in the
>> new page tables. If there is a global TLB entry for that in its prior
>> read-only state, it definitely needs to be flushed before attempting to
>> write through that virtual mapping.
>> 
>> It would be possible to just avoid writing to the virtual address of the
>> page and defer all writes until they can be done through the identity
>> mapping. But there's no good reason to keep the old TLB entries around,
>> as they can cause nothing but trouble.
>> 
>> Clear the PGE bit in %cr4 early, before storing data in the control page.
>
>It worth noting that flipping CR4.PGE triggers TLB flush. I was not sure
>if CR3 write is required to make it happen.


Well, until we flip to the new CR3 the read-only PTE can just get reloaded. But after CR4.PGE is cleared, of course they won't be global any more. So they will get flushed (again) when CR3 is reloaded.

Maybe it could run a tiny bit faster if we change CR3 before CR4? I don't know that we care about microbenchmarking kexec to that degree, but I may take a look...

Re: [PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[Dave Hansen]

On 12/17/24 04:25, Kirill A. Shutemov wrote:
>> Clear the PGE bit in %cr4 early, before storing data in the control page.
> It worth noting that flipping CR4.PGE triggers TLB flush. I was not sure
> if CR3 write is required to make it happen.

I thought about removing the CR3 write. But I decided against it because
CR4.PGE needs to actually change value, unlike CR3 writes where any
write can flush the TLB (modulo globals, PCID and bit 63 of course).

X86_FEATURE_PGE itself is required but I couldn't actually remember if
there are any cases where CR4.PGE==0. If there were, the CR3 write would
still be needed. I don't _think_ there are any ways forx86_64 to end up
with CR4.PGE==0, but I also wouldn't out the possibility that some silly
issue pops up making us play stupid games and win stupid prizes.

Anyway, I think we can leave the belt-and-suspenders programming in this
case. A comment wouldn't hurt I guess.

Re: [EXTERNAL] [PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

On Tue, 2024-12-17 at 06:51 -0800, Dave Hansen wrote:
> On 12/17/24 04:25, Kirill A. Shutemov wrote:
> > > Clear the PGE bit in %cr4 early, before storing data in the control page.
> > It worth noting that flipping CR4.PGE triggers TLB flush. I was not sure
> > if CR3 write is required to make it happen.
> 
> I thought about removing the CR3 write. But I decided against it because
> CR4.PGE needs to actually change value, unlike CR3 writes where any
> write can flush the TLB (modulo globals, PCID and bit 63 of course).
> 
> X86_FEATURE_PGE itself is required but I couldn't actually remember if
> there are any cases where CR4.PGE==0. If there were, the CR3 write would
> still be needed. I don't _think_ there are any ways forx86_64 to end up
> with CR4.PGE==0, but I also wouldn't out the possibility that some silly
> issue pops up making us play stupid games and win stupid prizes.
> 
> Anyway, I think we can leave the belt-and-suspenders programming in this
> case. A comment wouldn't hurt I guess.

I'm a little lost. In this case I don't see belt-and-suspenders
programming. We're not loading CR3 after clearing CR4.PGE just to be
paranoid about making really really sure the TLB is flushed.

We're loading CR3 because we're switching from the kernel's page tables
to the new identity mapping set up for the relocate_kernel environment.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [EXTERNAL] [PATCH 1/9] x86/kexec: Disable global pages before writing to control page

[Dave Hansen]

On 12/17/24 06:56, David Woodhouse wrote:
>> Anyway, I think we can leave the belt-and-suspenders programming in this
>> case. A comment wouldn't hurt I guess.
> I'm a little lost. In this case I don't see belt-and-suspenders
> programming. We're not loading CR3 after clearing CR4.PGE just to be
> paranoid about making really really sure the TLB is flushed.
> 
> We're loading CR3 because we're switching from the kernel's page tables
> to the new identity mapping set up for the relocate_kernel environment.

Yes, agreed, that's another reason the CR3 write must stay. I hadn't
even considered that part yet honestly.

Re: [PATCH 2/9] x86/kexec: Ensure preserve_context flag is set on return to kernel

[Uros Bizjak]

On 17. 12. 24 00:24, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The swap_pages function will only actually *swap*, as its name implies,
> if the preserve_context flag in the %r11 register is non-zero. On the
> way back from a ::preserve_context kexec, ensure that the %r11 register
> is non-zero so that the pages get swapped back.
> 
> Fixes: 9e5683e2d0b5 ("x86/kexec: Only swap pages for ::preserve_context mode")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   arch/x86/kernel/relocate_kernel_64.S | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 9bd601dd8659..1a52e4339c1d 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -220,6 +220,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>   	movq	kexec_pa_table_page(%rip), %rax
>   	movq	%rax, %cr3
>   	lea	PAGE_SIZE(%r8), %rsp
> +	movq	$1, %r11	/* Ensure preserve_context flag is set */

You can save a byte here by using "movl $1, %r11d".

>   	call	swap_pages
>   	movq	kexec_va_control_page(%rip), %rax
>   	addq	$(virtual_mapped - relocate_kernel), %rax

Uros.

Re: [PATCH 0/9] x86/kexec: Fixes for tip/x86/boot

[David Woodhouse]

On 16 December 2024 23:24:07 GMT, David Woodhouse <dwmw2@infradead.org> wrote:
>Fix a few bugs introduced by the recent rework of the relocate_kernel()
>code, and one which has been there in the kjump code from the beginning.
>
> • Global read-only TLB entries made relocate_kernel() crash when writing
>   to its own page after the %cr3 switch should have made it writeable.
> • The preserve_context flag was lost when invoking swap_pages on the way
>   back, causing pages not to be swapped.
> • The wrong page was being used as the swap page.
> • The ABI for the kjump call asks the callee to put its entry address for
>   next time at %rsp+8 before returning, but we set %rsp to the top of the
>   available page, such that the entry address is at the start of some
>   other page.
> • The relocate_kernel() function lacked Clang CFI information, but is now
>   called via a function pointer, leading to a crash¹.
> • The relocate_kernel() code and data could end up being linked into the
>   wrong place for a LTO / -ffunction-sections build.
>
>Thanks to Nathan for reporting many of the above.
>
>Also a few minor cleanups, including a comments-only patch from Rafael 
>on the suspend-like part of kjump as a prelude to actually cleaning that 
>up.
>
>¹ Fixed by just adding __nocfi. Actually providing the CFI information 
>for relocate_kernel() will need a bit more work, so let's just do the 
>simple fix for now.
>
>David Woodhouse (7):
>      x86/kexec: Disable global pages before writing to control page
>      x86/kexec: Ensure preserve_context flag is set on return to kernel
>      x86/kexec: Use correct swap page in swap_pages function
>      x86/kexec: Fix stack and handling of re-entry point for ::preserve_context
>      x86/kexec: Mark machine_kexec() with __nocfi
>      x86/kexec: Cope with relocate_kernel() not being at the start of the page
>      x86/kexec: Use typedef for relocate_kernel_fn function prototype
>
>Nathan Chancellor (1):
>      x86/kexec: Fix location of relocate_kernel with -ffunction-sections
>
>Rafael J. Wysocki (1):
>      kexec_core: Add and update comments regarding the KEXEC_JUMP flow
>
> arch/x86/include/asm/kexec.h         | 26 ++++++++--------
> arch/x86/kernel/machine_kexec_32.c   |  7 +----
> arch/x86/kernel/machine_kexec_64.c   |  8 ++---
> arch/x86/kernel/relocate_kernel_64.S | 57 ++++++++++++++++++++++++------------
> arch/x86/kernel/vmlinux.lds.S        |  4 +--
> kernel/kexec_core.c                  | 23 +++++++++++----
> 6 files changed, 74 insertions(+), 51 deletions(-)
>
>

Ping? I made sure I got this series out quickly as it deals with regressions in tip/x86/boot. Aside from potentially being able to save a byte on one instruction, I think the rest of the discussion has been resolved without changing the patches.

I'll resend the follow-on parts which add the debugging support when I'm back at a real keyboard.

Re: [PATCH 7/9] kexec_core: Add and update comments regarding the KEXEC_JUMP flow

[Dave Young]

> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index c0caa14880c3..7cf8437e0f38 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1001,6 +1001,12 @@ int kernel_kexec(void)
>
>  #ifdef CONFIG_KEXEC_JUMP
>         if (kexec_image->preserve_context) {
> +               /*
> +                * This flow is analogous to hibernation flows that occur before
> +                * creating an image and before jumping from the restore kernel
> +                * to the image one, so it uses the same device device callbacks

nitpick: s/device device/device

> +                * as those two flows.
> +                */
>                 pm_prepare_console();
>                 error = freeze_processes();
>                 if (error) {

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Tue, 2024-12-17 at 10:25 +0100, Ard Biesheuvel wrote:
> 
> > > You can still avoid the absolute relocations though, ...
> > ...
> > > > +       addq    $identity_mapped, %rsi
> > > > +       subq    $__relocate_kernel_start, %rsi
> > > 
> > > ... if you turn this into
> > > 
> > > 0:     addq    $identity_mapped - 0b, %rsi
> > >        subq    $__relocate_kernel_start - 0b, %rsi
> > 
> > Is there any benefit to doing so? Are absolute relocations problematic?
> 
> Every absolute relocation produces an entry in the relocation table
> that needs to be applied at every boot when KASLR is in effect. Beyond
> that, it doesn't matter.
> 
> I've looked into PIC codegen/PIE linking for the core kernel, which is
> why this caught my eye. If that effort ever advances, I'll need to
> revisit this code as well and apply the change I suggested.

OK, since it looks like I'll be reposting this series once I'm back at
a keyboard for real, I've done that in my tree.

There's one more absolute relocation, for saved_context just before
returning to the kernel from the 'virtual_mapped' code. That's only
reloading the GDT, and we could probably do that from the C code in
machine_kexec().

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH 7/9] kexec_core: Add and update comments regarding the KEXEC_JUMP flow

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Fri, 2025-01-03 at 17:24 +0800, Dave Young wrote:
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index c0caa14880c3..7cf8437e0f38 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -1001,6 +1001,12 @@ int kernel_kexec(void)
> > 
> >  #ifdef CONFIG_KEXEC_JUMP
> >         if (kexec_image->preserve_context) {
> > +               /*
> > +                * This flow is analogous to hibernation flows that occur before
> > +                * creating an image and before jumping from the restore kernel
> > +                * to the image one, so it uses the same device device callbacks
> 
> nitpick: s/device device/device

Thanks. Fixed up locally and will be in the resend.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH 2/9] x86/kexec: Ensure preserve_context flag is set on return to kernel

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Tue, 2024-12-17 at 17:38 +0100, Uros Bizjak wrote:
>  
> > diff --git a/arch/x86/kernel/relocate_kernel_64.S
> > b/arch/x86/kernel/relocate_kernel_64.S
> > index 9bd601dd8659..1a52e4339c1d 100644
> > --- a/arch/x86/kernel/relocate_kernel_64.S
> > +++ b/arch/x86/kernel/relocate_kernel_64.S
> > @@ -220,6 +220,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> >    	movq	kexec_pa_table_page(%rip), %rax
> >    	movq	%rax, %cr3
> >    	lea	PAGE_SIZE(%r8), %rsp
> > +	movq	$1, %r11	/* Ensure preserve_context flag is
> > set */
> 
> You can save a byte here by using "movl $1, %r11d".

Thanks. I've done that locally.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[Ard Biesheuvel]

On Fri, 3 Jan 2025 at 11:10, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Tue, 2024-12-17 at 10:25 +0100, Ard Biesheuvel wrote:
> >
> > > > You can still avoid the absolute relocations though, ...
> > > ...
> > > > > +       addq    $identity_mapped, %rsi
> > > > > +       subq    $__relocate_kernel_start, %rsi
> > > >
> > > > ... if you turn this into
> > > >
> > > > 0:     addq    $identity_mapped - 0b, %rsi
> > > >        subq    $__relocate_kernel_start - 0b, %rsi
> > >
> > > Is there any benefit to doing so? Are absolute relocations problematic?
> >
> > Every absolute relocation produces an entry in the relocation table
> > that needs to be applied at every boot when KASLR is in effect. Beyond
> > that, it doesn't matter.
> >
> > I've looked into PIC codegen/PIE linking for the core kernel, which is
> > why this caught my eye. If that effort ever advances, I'll need to
> > revisit this code as well and apply the change I suggested.
>
> OK, since it looks like I'll be reposting this series once I'm back at
> a keyboard for real, I've done that in my tree.
>

Thanks

> There's one more absolute relocation, for saved_context just before
> returning to the kernel from the 'virtual_mapped' code. That's only
> reloading the GDT, and we could probably do that from the C code in
> machine_kexec().

I suppose you're referring to

#ifdef CONFIG_KEXEC_JUMP
        /* Saved in save_processor_state. */
        movq    $saved_context, %rax
        lgdt    saved_context_gdt_desc(%rax)
#endif

Any reason not to simply use

        lgdt    saved_context+saved_context_gdt_desc(%rip)

here?

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Mon, 2025-01-06 at 17:09 +0100, Ard Biesheuvel wrote:
> 
> I suppose you're referring to
> 
> #ifdef CONFIG_KEXEC_JUMP
>         /* Saved in save_processor_state. */
>         movq    $saved_context, %rax
>         lgdt    saved_context_gdt_desc(%rax)
> #endif
> 
> Any reason not to simply use
> 
>         lgdt    saved_context+saved_context_gdt_desc(%rip)
> 
> here?

Because the %rip isn't what you (and the linker) think it is.

This code is copied into a control page which is allocated as part of
the kexec kimage. It can only access things within that same page via
%rip. (Which is not as much of a restriction as it sounds, as for most
of its execution the rest of the kernel isn't even present in the page
tables).

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH 8/9] x86/kexec: Cope with relocate_kernel() not being at the start of the page

[Ard Biesheuvel]

On Mon, 6 Jan 2025 at 17:13, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Mon, 2025-01-06 at 17:09 +0100, Ard Biesheuvel wrote:
> >
> > I suppose you're referring to
> >
> > #ifdef CONFIG_KEXEC_JUMP
> >         /* Saved in save_processor_state. */
> >         movq    $saved_context, %rax
> >         lgdt    saved_context_gdt_desc(%rax)
> > #endif
> >
> > Any reason not to simply use
> >
> >         lgdt    saved_context+saved_context_gdt_desc(%rip)
> >
> > here?
>
> Because the %rip isn't what you (and the linker) think it is.
>
> This code is copied into a control page which is allocated as part of
> the kexec kimage. It can only access things within that same page via
> %rip. (Which is not as much of a restriction as it sounds, as for most
> of its execution the rest of the kernel isn't even present in the page
> tables).

Ah I was looking at an older version of that file - I now see that the
preceding (%rip) references have been replaced as well.

In any case, thanks for the head's up - I'll get back to this at some
point and cc you if making any further changes here.