kexec: Fix CMA and elf header leaks and rework
v7 cover kexec: Fix CMA and elf header leaks and rework
ARM64, LoongArch and RISC-V load the elf core header, initrd, DTB and cmdline parameters with almost identical code. This series fixes the bugs in that path. And as Mike suggested, rework the common part into a helper.
Patch 1 stops using the Image text_offset for arm64, which breaks CMA segments in both the load and release paths, and rejects images with a non-zero value.
Patches 2-7 fix the CMA page and elf core header leaks in the retry loops in arm64 and LoongArch, also fix a modified_cmdline leak in LoongArch.
Patches 8-10 pass the kbuf fields to crash_prepare_headers() to simplfy code.
Patch 11 frees the elf core header from the generic post-load cleanup.
Patches 12-15 add kexec_load_other_segments() and convert the three architectures, and arch_kexec_setup_other_segments() builds the architecture specific boot parameters (a DTB or a command line).
Tested with kexec_file_load() and kdump on arm64, RISC-V and LoongArch under QEMU, including the non-zero text_offset rejection.
This series is also available at:
https://github.com/ruanjinjie-eng/linux/tree/kexec_cleanup_v7
Changes in v7:
- Rework the kexec_file load code to avoid code duplication
as Mike suggested.
- Add Acked-by.
- Drop the two non-zero text_offset fix patches
and deprecate .text_offset in arm64 as Mike suggested.
- Drop the x86-32 fix patch.
- Drop the TOCTOU race fix patches.
- Link to v6: https://lore.kernel.org/all/[email protected]/
Previous Versions are as below:
- v5: https://lore.kernel.org/all/[email protected]/
- v4: https://lore.kernel.org/all/[email protected]/
- v3: https://lore.kernel.org/all/[email protected]/
- v2: https://lore.kernel.org/all/[email protected]/
- v1: https://lore.kernel.org/all/[email protected]/#t
Jinjie Ruan (15):
arm64: kexec_file: Stop using the Image text_offset
kexec: Extract kexec_free_segment_cma() from kimage_free_cma()
arm64: kexec_file: Fix CMA page leaks in segment placement retry loops
arm64: kexec_file: Fix elf_headers memory leak in retry loop
LoongArch: kexec_file: Fix CMA page leaks in segment placement retry
loops
LoongArch: kexec_file: Fix elf_headers memory leak in retry loop
LoongArch: kexec_file: Fix a modified_cmdline leak
arm64: kexec_file: Simplify load_other_segments()
LoongArch: kexec_file: Simplify load_other_segments()
riscv: kexec_file: Simplify load_extra_segments()
kexec: Free the elf core header in kimage_file_post_load_cleanup()
kexec: Add kexec_load_other_segments() helper
arm64: kexec_file: Use kexec_load_other_segments() to simplify code
LoongArch: kexec_file: Use kexec_load_other_segments() to simplify
code
riscv: kexec_file: Use kexec_load_other_segments() to simplify code
arch/arm64/include/asm/kexec.h | 5 - arch/arm64/kernel/kexec_image.c | 33 +++++-- arch/arm64/kernel/machine_kexec_file.c | 103 ++++----------------- arch/loongarch/include/asm/kexec.h | 4 - arch/loongarch/kernel/kexec_efi.c | 20 +++- arch/loongarch/kernel/kexec_elf.c | 11 ++- arch/loongarch/kernel/machine_kexec.c | 2 + arch/loongarch/kernel/machine_kexec_file.c | 96 +++---------------- arch/powerpc/kexec/file_load_64.c | 4 - arch/riscv/include/asm/kexec.h | 4 - arch/riscv/kernel/kexec_elf.c | 11 ++- arch/riscv/kernel/kexec_image.c | 11 ++- arch/riscv/kernel/machine_kexec_file.c | 79 ++++------------ arch/x86/kernel/machine_kexec_64.c | 4 - include/linux/kexec.h | 17 ++++ kernel/kexec_core.c | 25 ++--- kernel/kexec_file.c | 99 ++++++++++++++++++++ 17 files changed, 248 insertions(+), 280 deletions(-)
Signature · 1 line
2.34.1
PATCH v7 1/15 arm64: kexec_file: Stop using the Image text_offset
image_load() adds text_offset to kernel_segment->mem and subtracts it from kernel_segment->memsz. For CMA backed segments this breaks the load path: the payload is copied to page_address(cma), which is the CMA base, while image->start is set to the adjusted address, so kexec jumps into the middle of the kernel. kimage_map_segment() has the same mismatch when a segment has been moved.
The adjusted memsz also breaks the release path: the CMA pages are freed using segment->memsz >> PAGE_SHIFT, which is fewer than were allocated, leaking the tail of the allocation.
Since commit 751c263bb74f ("arm64: remove TEXT_OFFSET randomization") TEXT_OFFSET is always zero. This has been upstream since v5.8, so no LTS kernel carries a non-zero text_offset. Drop the adjustment and reject images with a non-zero value instead of booting them from a wrong address. With the adjustment gone, segments stay at their CMA base and the release size matches the allocation.
--- arch/arm64/kernel/kexec_image.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
arch/arm64/kernel/kexec_image.c +8 -5
@@ -80,12 +80,17 @@ static void *image_load(struct kimage *image, kbuf.bufsz = kernel_len; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; kbuf.memsz =le64_to_cpu (h->image_size); +/* +* Pre-v5.8 images carry a non-zero text_offset, which is no longer +* honored; reject them instead of loading them at a wrong address. +*/ text_offset =le64_to_cpu (h->text_offset); +if (text_offset) { +pr_err ("Unsupported Image text_offset 0x%lx\n" , text_offset); +return ERR_PTR (-ENOEXEC ); + } kbuf.buf_align =MIN_KIMG_ALIGN ; -/* Adjust kernel segment with TEXT_OFFSET */ - kbuf.memsz += text_offset; - kernel_segment_number = image->nr_segments;/*
@@ -117,8 +122,6 @@ static void *image_load(struct kimage *image, } kernel_segment = &image->segment[kernel_segment_number]; - kernel_segment->mem += text_offset; - kernel_segment->memsz -= text_offset; image->start = kernel_segment->mem; kexec_dprintk("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 01/15] arm64: kexec_file: Stop using the Image text_offset
Sashiko has reviewed this patch and found no issues.
Quoted 18 lines
image_load()addstext_offsettokernel_segment->memand subtracts it fromkernel_segment->memsz. For CMA backed segments this breaks the load path: the payload is copied topage_address(cma), which is the CMA base, whileimage->startis set to the adjusted address, so kexec jumps into the middle of the kernel.kimage_map_segment()has the same mismatch when a segment has been moved.The adjusted memsz also breaks the release path: the CMA pages are freed using
segment->memsz>>PAGE_SHIFT, which is fewer than were allocated, leaking the tail of the allocation.Since commit
751c263bb74f("arm64: removeTEXT_OFFSETrandomization")TEXT_OFFSETis always zero. This has been upstream since v5.8, so no LTS kernel carries a non-zerotext_offset. Drop the adjustment and reject images with a non-zero value instead of booting them from a wrong address. With the adjustment gone, segments stay at their CMA base and the release size matches the allocation.
Quoted 8 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mike Rapoport <[email protected]>Fixes 07d24902977e ("kexec: enable CMA based contiguous allocation")Suggested-by Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v7 2/15 kexec: Extract kexec_free_segment_cma() from kimage_free_cma()
In reply to Jinjie RuanArch loaders truncate nr_segments when a placement fails, so the CMA pages of the dropped segments become unreachable. Export a per-segment helper so they can be released first.
--- include/linux/kexec.h | 2 ++ kernel/kexec_core.c | 25 ++++++++++++++----------- 2 files changed, 16 insertions(+), 11 deletions(-)
include/linux/kexec.h +2 -0
@@ -532,6 +532,7 @@ extern bool kexec_file_dbg_print;extern void *kimage_map_segment (struct kimage *image,int idx);extern void kimage_unmap_segment (void *buffer); +void kexec_free_segment_cma (struct kimage *image,unsigned long idx);#else /* !CONFIG_KEXEC_CORE */ struct pt_regs ;struct task_struct ;
@@ -543,6 +544,7 @@ static inline int kexec_crash_loaded(void) { return 0; }static inline void *kimage_map_segment (struct kimage *image,int idx) {return NULL ; }static inline void kimage_unmap_segment (void *buffer) { } +static inline void kexec_free_segment_cma (struct kimage *image,unsigned long idx) { }#define kexec_in_progress false#endif /* CONFIG_KEXEC_CORE */
kernel/kexec_core.c +14 -11
@@ -554,22 +554,25 @@ static void kimage_free_entry(kimage_entry_t entry)kimage_free_pages (page); } -static void kimage_free_cma (struct kimage *image) +void kexec_free_segment_cma (struct kimage *image,unsigned long idx) { -unsigned long i; +struct page *cma = image->segment_cma[idx]; +u32 nr_pages = image->segment[idx].memsz >>PAGE_SHIFT ; -for (i =0 ; i < image->nr_segments; i++) { -struct page *cma = image->segment_cma[i]; -u32 nr_pages = image->segment[i].memsz >>PAGE_SHIFT ; +if (!cma) +return ; -if (!cma) -continue ; +arch_kexec_pre_free_pages (page_address (cma), nr_pages); +dma_release_from_contiguous (NULL , cma, nr_pages); + image->segment_cma[idx] =NULL ; +} -arch_kexec_pre_free_pages (page_address (cma), nr_pages); -dma_release_from_contiguous (NULL , cma, nr_pages); - image->segment_cma[i] =NULL ; - } +static void kimage_free_cma (struct kimage *image) +{ +unsigned long i; +for (i =0 ; i < image->nr_segments; i++) +kexec_free_segment_cma (image, i); }void kimage_free (struct kimage *image)
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 02/15] kexec: Extract kexec_free_segment_cma() from kimage_free_cma()
Sashiko has reviewed this patch and found no issues.
Quoted 11 lines
Arch loaders truncate
nr_segmentswhen a placement fails, so the CMA pages of the dropped segments become unreachable. Export a per-segment helper so they can be released first.Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Cc Pasha Tatashin <[email protected]>Cc Pratyush Yadav <[email protected]>Cc Breno Leitao <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v7 3/15 arm64: kexec_file: Fix CMA page leaks in segment placement retry loops
In reply to Jinjie RuanThe retry loop drops nr_segments, leaking the CMA pages of the failed segments. Release them first, and disable CMA for the retry: it ignores buf_min and would hand out the same region forever.
--- arch/arm64/kernel/kexec_image.c | 8 ++++++++ arch/arm64/kernel/machine_kexec_file.c | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-)
arch/arm64/kernel/kexec_image.c +8 -0
@@ -111,6 +111,14 @@ static void *image_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/* +* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;
arch/arm64/kernel/machine_kexec_file.c +4 -1
@@ -179,7 +179,10 @@ int load_other_segments(struct kimage *image,return 0 ; out_err: - image->nr_segments = orig_segments; +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--; + }kvfree (dtb);return ret; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 03/15] arm64: kexec_file: Fix CMA page leaks in segment placement retry loops
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] arm64: kexec_file: image_load() skips RAM below CMA on non-CMA retry --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: kexec_file: image_load() skips RAM below CMA on non-CMA retry --
commit a177040c2457cfc0cd78f4ced8850ba0e9322a44
Author: Jinjie Ruan <[email protected]>
arm64: kexec_file: Fix CMA page leaks in segment placement retry loops
This commit frees CMA-backed segments when rewinding image->nr_segments in image_load() and load_other_segments(), and sets image->no_cma when a CMA-allocated kernel segment placement retry occurs.
Quoted 9 lines
arch/arm64/kernel/kexec_image.c +1
-0@@ -111,6 +111,14 @@ static void *image_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/*
arch/arm64/kernel/kexec_image.c static void *image_load(struct kimage *image, · line 124
Quoted 10 lines
arch/arm64/kernel/kexec_image.c +7
-0+* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;
[Severity: Medium]
When falling back from a CMA allocation to a non-CMA search in image_load(), does unconditionally advancing kbuf.buf_min to kernel_segment->mem + kernel_segment->memsz skip all free RAM below the CMA region?
In image_load(), kbuf.buf_min starts at 0 with kbuf.top_down set to false:
arch/arm64/kernel/kexec_image.c:image_load() {
kbuf.image = image;
kbuf.buf_min = 0;
kbuf.buf_max = ULONG_MAX;
kbuf.top_down = false;
...
while ((ret = kexec_add_buffer(&kbuf)) == 0) {On the first iteration, kexec_add_buffer() -> kexec_locate_mem_hole() calls kexec_alloc_contig() before walking memblock:
kernel/kexec_file.c:kexec_locate_mem_hole() {
if (!kexec_alloc_contig(kbuf))
return 0;
if (!IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
ret = kexec_walk_resources(kbuf, locate_mem_hole_callback);
else
ret = kexec_walk_memblock(kbuf, locate_mem_hole_callback);Because kexec_alloc_contig() allocates from the CMA area without consulting kbuf.buf_min, the bottom-up walk starting from 0 has not run yet.
If load_other_segments() then fails to place initrd or dtb above kernel_segment->mem + kernel_segment->memsz (for example, when CMA is near the top of RAM and lacks space for the remaining segments), setting image->no_cma = true and advancing kbuf.buf_min to kernel_segment->mem + kernel_segment->memsz causes the next iteration's kexec_walk_memblock() to search only above the CMA region, where load_other_segments() already failed to find space.
Should kbuf.buf_min only be updated to kernel_segment->mem + kernel_segment->memsz when the failed kernel segment was not allocated from CMA?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH v7 03/15] arm64: kexec_file: Fix CMA page leaks in segment placement retry loops
In reply to Jinjie RuanWill fix sashiko review issue in v8.
Quoted 54 lines · Jinjie Ruan
The retry loop drops
nr_segments, leaking the CMA pages of the failed segments. Release them first, and disable CMA for the retry: it ignoresbuf_minand would hand out the same region forever.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Breno Leitao <[email protected]>Cc Pratyush Yadav <[email protected]>Cc Andrew Morton <[email protected]>Cc Yeoreum Yun <[email protected]>Cc Baoquan He <[email protected]>Fixes 07d24902977e ("kexec: enable CMA based contiguous allocation")Signed-off-by Jinjie Ruan <[email protected]>--- arch/arm64/kernel/kexec_image.c | 8 ++++++++ arch/arm64/kernel/machine_kexec_file.c | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-)arch/arm64/kernel/kexec_image.c +8
-0@@ -111,6 +111,14 @@ static void *image_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/* +* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;arch/arm64/kernel/machine_kexec_file.c +4
-1@@ -179,7 +179,10 @@ int load_other_segments(struct kimage *image,return 0 ; out_err: - image->nr_segments = orig_segments; +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--; + }kvfree (dtb);return ret; }
Signature · 2 lines
Best regards, Jinjie
PATCH v7 4/15 arm64: kexec_file: Fix elf_headers memory leak in retry loop
In reply to Jinjie RuanA retry allocates new elf headers and overwrites image->elf_headers, leaking the previous buffer. Free it before assigning.
--- arch/arm64/kernel/machine_kexec_file.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm64/kernel/machine_kexec_file.c +4 -0
@@ -112,6 +112,10 @@ int load_other_segments(struct kimage *image,vfree (headers); goto out_err; } + +if (unlikely (image->elf_headers)) +vfree (image->elf_headers); + image->elf_headers = headers; image->elf_load_addr = kbuf.mem; image->elf_headers_sz = headers_sz;
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 04/15] arm64: kexec_file: Fix elf_headers memory leak in retry loop
Sashiko has reviewed this patch and found no issues.
Quoted 14 lines
A retry allocates new elf headers and overwrites
image->elf_headers, leaking the previous buffer. Free it before assigning.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Thomas Huth <[email protected]>Cc Breno Leitao <[email protected]>Cc Andrew Morton <[email protected]>Cc Yeoreum Yun <[email protected]>Cc Baoquan He <[email protected]>Fixes 108aa503657e ("arm64: kexec_file: try more regions if loading segments fails")Acked-by Catalin Marinas <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v7 5/15 LoongArch: kexec_file: Fix CMA page leaks in segment placement retry loops
In reply to Jinjie RuanThe retry loop drops nr_segments, leaking the CMA pages of the failed segments. Release them first, and disable CMA for the retry: it ignores buf_min and would hand out the same region forever.
--- arch/loongarch/kernel/kexec_efi.c | 8 ++++++++ arch/loongarch/kernel/machine_kexec_file.c | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-)
arch/loongarch/kernel/kexec_efi.c +8 -0
@@ -86,6 +86,14 @@ static void *efi_kexec_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/* +* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;
arch/loongarch/kernel/machine_kexec_file.c +5 -1
@@ -217,7 +217,11 @@ int load_other_segments(struct kimage *image,return 0 ; out_err: - image->nr_segments = orig_segments; +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--; + } +kfree (modified_cmdline);return ret; }
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 05/15] LoongArch: kexec_file: Fix CMA page leaks in segment placement retry loops
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] LoongArch: kexec_file: efi_kexec_load() skips RAM below CMA on retry --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] LoongArch: kexec_file: efi_kexec_load() skips RAM below CMA on retry --
commit f502069414c135fd39705a4caf6f9bdd9bb1a976
Author: Jinjie Ruan <[email protected]>
LoongArch: kexec_file: Fix CMA page leaks in segment placement retry loops
This commit fixes CMA page leaks in efi_kexec_load() and load_other_segments() when segment placement fails, and disables CMA on retry so kexec_add_buffer() does not repeatedly allocate the same CMA region.
Quoted 9 lines
arch/loongarch/kernel/kexec_efi.c +1
-0@@ -86,6 +86,14 @@ static void *efi_kexec_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/*
arch/loongarch/kernel/kexec_efi.c static void *efi_kexec_load(struct kimage *image, · line 99
Quoted 10 lines
arch/loongarch/kernel/kexec_efi.c +7
-0+* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;
[Severity: Medium]
When falling back from a CMA allocation to a normal bottom-up System RAM search in efi_kexec_load(), does advancing kbuf.buf_min skip valid System RAM located below the CMA region?
Before the retry loop in efi_kexec_load(), kbuf is set up for a bottom-up search starting at text_offset:
kbuf.top_down = false;
...
kbuf.buf_min = text_offset;On the first iteration, kexec_add_buffer() -> kexec_locate_mem_hole() -> kexec_alloc_contig() allocates the kernel segment from CMA, which ignores kbuf.buf_min and may place the kernel high in physical memory (for example, near the top of DRAM or above 4GB).
If load_other_segments() then fails to place the initrd because there is not enough space above the CMA kernel segment, the retry path sets image->no_cma = true so the next iteration falls back to kexec_walk_memblock(). However, it still updates kbuf.buf_min:
kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz;
Because the first attempt came from CMA rather than a bottom-up walk from text_offset, won't advancing kbuf.buf_min past the CMA allocation skip all System RAM in [text_offset, kernel_segment->mem + kernel_segment->memsz) and cause kexec_file_load() to fail with -EADDRNOTAVAIL?
Should kbuf.buf_min only be advanced when the failed kernel segment was not allocated from CMA?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Re: [PATCH v7 05/15] LoongArch: kexec_file: Fix CMA page leaks in segment placement retry loops
In reply to Jinjie RuanWill fix sashiko review issue in v8.
Quoted 55 lines · Jinjie Ruan
The retry loop drops
nr_segments, leaking the CMA pages of the failed segments. Release them first, and disable CMA for the retry: it ignoresbuf_minand would hand out the same region forever.Cc Huacai Chen <[email protected]>Cc WANG Xuerui <[email protected]>Cc Youling Tang <[email protected]>Cc Mike Rapoport (Microsoft) <[email protected]>Cc Sourabh Jain <[email protected]>Cc Kees Cook <[email protected]>Fixes 55d990f0084c ("LoongArch: Add EFI binary support for kexec_file")Signed-off-by Jinjie Ruan <[email protected]>--- arch/loongarch/kernel/kexec_efi.c | 8 ++++++++ arch/loongarch/kernel/machine_kexec_file.c | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-)arch/loongarch/kernel/kexec_efi.c +8
-0@@ -86,6 +86,14 @@ static void *efi_kexec_load(struct kimage *image,* We couldn't find space for the other segments; erase the * kernel segment and try the next available hole. */ +/* +* kexec_alloc_contig() ignores buf_min, so disable CMA to +* let the retry make progress. +*/ +if (image->segment_cma[kernel_segment_number]) + image->no_cma = true; + +kexec_free_segment_cma (image, kernel_segment_number); image->nr_segments -=1 ; kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz; kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ;arch/loongarch/kernel/machine_kexec_file.c +5
-1@@ -217,7 +217,11 @@ int load_other_segments(struct kimage *image,return 0 ; out_err: - image->nr_segments = orig_segments; +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--; + } +kfree (modified_cmdline);return ret; }
Signature · 2 lines
Best regards, Jinjie
PATCH v7 6/15 LoongArch: kexec_file: Fix elf_headers memory leak in retry loop
In reply to Jinjie RuanA retry allocates new elf headers and overwrites image->elf_headers, leaking the previous buffer. Free it before assigning.
--- arch/loongarch/kernel/machine_kexec_file.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/loongarch/kernel/machine_kexec_file.c +4 -0
@@ -166,6 +166,10 @@ int load_other_segments(struct kimage *image,vfree (headers); goto out_err; } + +if (unlikely (image->elf_headers)) +vfree (image->elf_headers); + image->elf_headers = headers; image->elf_load_addr = kbuf.mem; image->elf_headers_sz = headers_sz;
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 06/15] LoongArch: kexec_file: Fix elf_headers memory leak in retry loop
Sashiko has reviewed this patch and found no issues.
Quoted 13 lines
A retry allocates new elf headers and overwrites
image->elf_headers, leaking the previous buffer. Free it before assigning.Cc Huacai Chen <[email protected]>Cc WANG Xuerui <[email protected]>Cc Youling Tang <[email protected]>Cc Mike Rapoport (Microsoft) <[email protected]>Cc Sourabh Jain <[email protected]>Cc Kees Cook <[email protected]>Fixes 55d990f0084c ("LoongArch: Add EFI binary support for kexec_file")Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v7 7/15 LoongArch: kexec_file: Fix a modified_cmdline leak
In reply to Jinjie Ruanmachine_kexec_prepare() copies it to KEXEC_CMDLINE_ADDR and overwrites the pointer, so free the heap buffer after the copy.
--- arch/loongarch/kernel/machine_kexec.c | 2 ++ 1 file changed, 2 insertions(+)
arch/loongarch/kernel/machine_kexec.c +2 -0
@@ -14,6 +14,7 @@#include <linux/mm.h> #include <linux/of_fdt.h> #include <linux/reboot.h> +#include <linux/slab.h> #include <linux/sched.h> #include <linux/sched/task_stack.h>
@@ -56,6 +57,7 @@ int machine_kexec_prepare(struct kimage *kimage)*/ memcpy ((void *)KEXEC_CMDLINE_ADDR , (void *)kimage->arch.cmdline_ptr,strlen ((char *)kimage->arch.cmdline_ptr) +1 ); +kfree ((void *)kimage->arch.cmdline_ptr); kimage->arch.cmdline_ptr = (unsigned long )KEXEC_CMDLINE_ADDR ; }else {/* Find the command line */
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 07/15] LoongArch: kexec_file: Fix a modified_cmdline leak
Sashiko has reviewed this patch and found no issues.
Quoted 13 lines
machine_kexec_prepare()copies it toKEXEC_CMDLINE_ADDRand overwrites the pointer, so free the heap buffer after the copy.Cc Huacai Chen <[email protected]>Cc WANG Xuerui <[email protected]>Cc Kexin Liu <[email protected]>Cc Youling Tang <[email protected]>Cc Qiang Ma <[email protected]>Cc Tianyang Zhang <[email protected]>Cc George Guo <[email protected]>Fixes d162feec6b6e ("LoongArch: Add preparatory infrastructure for kexec_file")Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v7 8/15 arm64: kexec_file: Simplify load_other_segments()
In reply to Jinjie RuanPass the kbuf fields to crash_prepare_headers() and set elf_headers before kexec_add_buffer(), so the error path needs no vfree() because arch_kimage_file_post_load_cleanup() will do it.
--- arch/arm64/kernel/machine_kexec_file.c | 35 +++++++++++--------------- 1 file changed, 15 insertions(+), 20 deletions(-)
arch/arm64/kernel/machine_kexec_file.c +15 -20
@@ -78,47 +78,42 @@ int load_other_segments(struct kimage *image, char *initrd, unsigned long initrd_len, char *cmdline) { - struct kexec_buf kbuf = {}; + struct kexec_buf kbuf = { + .image = image, +/* not allocate anything below the kernel */ + .buf_min = kernel_load_addr + kernel_size, + .buf_max =ULONG_MAX , + };void *dtb =NULL ;unsigned long initrd_load_addr =0 , dtb_len, orig_segments = image->nr_segments;int ret =0 ; - kbuf.image = image; -/* not allocate anything below the kernel */ - kbuf.buf_min = kernel_load_addr + kernel_size; - #ifdef CONFIG_CRASH_DUMP /* load elf core header */ - void *headers; - unsigned long headers_sz; if (image->type == KEXEC_TYPE_CRASH) { - ret = crash_prepare_headers(true, &headers, &headers_sz, NULL); + ret = crash_prepare_headers(true, &kbuf.buffer, &kbuf.bufsz, NULL); if (ret) { pr_err("Preparing elf core header failed\n"); goto out_err; } - kbuf.buffer = headers; - kbuf.bufsz = headers_sz; + if (unlikely(image->elf_headers)) + vfree(image->elf_headers); + + image->elf_headers = kbuf.buffer; + image->elf_headers_sz = kbuf.bufsz; + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN; - kbuf.memsz = headers_sz; + kbuf.memsz = kbuf.bufsz; kbuf.buf_align = SZ_64K; /* largest supported page size */ - kbuf.buf_max = ULONG_MAX; kbuf.top_down = true; ret = kexec_add_buffer(&kbuf); - if (ret) { - vfree(headers); + if (ret) goto out_err; - } - - if (unlikely(image->elf_headers)) - vfree(image->elf_headers); - image->elf_headers = headers; image->elf_load_addr = kbuf.mem; - image->elf_headers_sz = headers_sz; kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 08/15] arm64: kexec_file: Simplify load_other_segments()
Sashiko has reviewed this patch and found no issues.
Quoted 9 lines
Pass the kbuf fields to
crash_prepare_headers()and setelf_headersbeforekexec_add_buffer(), so the error path needs novfree()becausearch_kimage_file_post_load_cleanup()will do it.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Baoquan He <[email protected]>Cc Breno Leitao <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v7 9/15 LoongArch: kexec_file: Simplify load_other_segments()
In reply to Jinjie RuanPass the kbuf fields to crash_prepare_headers() and set elf_headers before kexec_add_buffer(), so the error path needs no vfree() because arch_kimage_file_post_load_cleanup() will do it.
--- arch/loongarch/kernel/machine_kexec_file.c | 38 +++++++++------------- 1 file changed, 16 insertions(+), 22 deletions(-)
arch/loongarch/kernel/machine_kexec_file.c +16 -22
@@ -127,11 +127,12 @@ int load_other_segments(struct kimage *image,unsigned long initrd_load_addr =0 ;unsigned long orig_segments = image->nr_segments;char *modified_cmdline =NULL ; -struct kexec_buf kbuf = {}; - - kbuf.image = image; -/* Don't allocate anything below the kernel */ - kbuf.buf_min = kernel_load_addr + kernel_size; +struct kexec_buf kbuf = { + .image = image, +/* Don't allocate anything below the kernel */ + .buf_min = kernel_load_addr + kernel_size, + .buf_max =ULONG_MAX , + }; modified_cmdline =kzalloc (COMMAND_LINE_SIZE ,GFP_KERNEL );if (!modified_cmdline)
@@ -144,35 +145,28 @@ int load_other_segments(struct kimage *image,#ifdef CONFIG_CRASH_DUMP /* Load elf core header */ if (image->type ==KEXEC_TYPE_CRASH ) { -void *headers; -unsigned long headers_sz; - - ret =crash_prepare_headers (true, &headers, &headers_sz,NULL ); + ret =crash_prepare_headers (true, &kbuf.buffer, &kbuf.bufsz,NULL );if (ret <0 ) {pr_err ("Preparing elf core header failed\n" ); goto out_err; } - kbuf.buffer = headers; - kbuf.bufsz = headers_sz; +if (unlikely (image->elf_headers)) +vfree (image->elf_headers); + + image->elf_headers = kbuf.buffer; + image->elf_headers_sz = kbuf.bufsz; + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; - kbuf.memsz = headers_sz; + kbuf.memsz = kbuf.bufsz; kbuf.buf_align =SZ_64K ;/* largest supported page size */ - kbuf.buf_max =ULONG_MAX ; kbuf.top_down = true; ret =kexec_add_buffer (&kbuf); -if (ret <0 ) { -vfree (headers); +if (ret <0 ) goto out_err; - } - -if (unlikely (image->elf_headers)) -vfree (image->elf_headers); - image->elf_headers = headers; image->elf_load_addr = kbuf.mem; - image->elf_headers_sz = headers_sz;kexec_dprintk ("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
@@ -181,7 +175,7 @@ int load_other_segments(struct kimage *image,cmdline_add_mem (&cmdline_tmplen, modified_cmdline);/* Add the elfcorehdr=size@start parameter to the command line */ - cmdline_add_elfcorehdr(image, &cmdline_tmplen, modified_cmdline, headers_sz); + cmdline_add_elfcorehdr(image, &cmdline_tmplen, modified_cmdline, kbuf.bufsz); }#endif
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 09/15] LoongArch: kexec_file: Simplify load_other_segments()
Sashiko has reviewed this patch and found no issues.
Quoted 7 lines
Pass the kbuf fields to
crash_prepare_headers()and setelf_headersbeforekexec_add_buffer(), so the error path needs novfree()becausearch_kimage_file_post_load_cleanup()will do it.Cc Huacai Chen <[email protected]>Cc Baoquan He <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
PATCH v7 10/15 riscv: kexec_file: Simplify load_extra_segments()
In reply to Jinjie RuanPass the kbuf fields to crash_prepare_headers() and set elf_headers before kexec_add_buffer(), so the error path needs no vfree() because arch_kimage_file_post_load_cleanup() will do it.
--- arch/riscv/kernel/machine_kexec_file.c | 28 +++++++++++--------------- 1 file changed, 12 insertions(+), 16 deletions(-)
arch/riscv/kernel/machine_kexec_file.c +12 -16
@@ -247,39 +247,35 @@ int load_extra_segments(struct kimage *image, unsigned long kernel_start,int ret;void *fdt;unsigned long initrd_pbase =0UL ; -struct kexec_buf kbuf = {};char *modified_cmdline =NULL ; - - kbuf.image = image; - kbuf.buf_min = kernel_start + kernel_len; - kbuf.buf_max =PFN_PHYS (max_low_pfn); +struct kexec_buf kbuf = { + .image = image, + .buf_min = kernel_start + kernel_len, + .buf_max =PFN_PHYS (max_low_pfn), + };#ifdef CONFIG_CRASH_DUMP /* Add elfcorehdr */ if (image->type ==KEXEC_TYPE_CRASH ) { -void *headers; -unsigned long headers_sz; - ret =crash_prepare_headers (true, &headers, &headers_sz,NULL ); + ret =crash_prepare_headers (true, &kbuf.buffer, &kbuf.bufsz,NULL );if (ret) {pr_err ("Preparing elf core header failed\n" ); goto out; } - kbuf.buffer = headers; - kbuf.bufsz = headers_sz; + image->elf_headers = kbuf.buffer; + image->elf_headers_sz = kbuf.bufsz; + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; - kbuf.memsz = headers_sz; + kbuf.memsz = kbuf.bufsz; kbuf.buf_align =ELF_CORE_HEADER_ALIGN ; kbuf.top_down = true; ret =kexec_add_buffer (&kbuf); -if (ret) { -vfree (headers); +if (ret) goto out; - } - image->elf_headers = headers; + image->elf_load_addr = kbuf.mem; - image->elf_headers_sz = headers_sz;kexec_dprintk ("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 10/15] riscv: kexec_file: Simplify load_extra_segments()
Sashiko has reviewed this patch and found no issues.
Quoted 8 lines
Pass the kbuf fields to
crash_prepare_headers()and setelf_headersbeforekexec_add_buffer(), so the error path needs novfree()becausearch_kimage_file_post_load_cleanup()will do it.Cc Palmer Dabbelt <[email protected]>Cc Alexandre Ghiti <[email protected]>Cc Baoquan He <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
PATCH v7 11/15 kexec: Free the elf core header in kimage_file_post_load_cleanup()
In reply to Jinjie RuanThe elf core header buffer is generic state, but arm64, LoongArch, RISC-V, x86 and powerpc each free it in their post-load cleanup. Move it into the generic cleanup, which also gives the loader a helper to release it on error.
--- arch/arm64/kernel/machine_kexec_file.c | 4 ---- arch/loongarch/kernel/machine_kexec_file.c | 4 ---- arch/powerpc/kexec/file_load_64.c | 4 ---- arch/riscv/kernel/machine_kexec_file.c | 4 ---- arch/x86/kernel/machine_kexec_64.c | 4 ---- kernel/kexec_file.c | 9 +++++++++ 6 files changed, 9 insertions(+), 20 deletions(-)
arch/arm64/kernel/machine_kexec_file.c +0 -4
@@ -32,10 +32,6 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)kvfree (image->arch.dtb); image->arch.dtb =NULL ; -vfree (image->elf_headers); - image->elf_headers =NULL ; - image->elf_headers_sz =0 ; -return kexec_image_post_load_cleanup_default (image); }
arch/loongarch/kernel/machine_kexec_file.c +0 -4
@@ -28,10 +28,6 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {int arch_kimage_file_post_load_cleanup (struct kimage *image) { -vfree (image->elf_headers); - image->elf_headers =NULL ; - image->elf_headers_sz =0 ; -return kexec_image_post_load_cleanup_default (image); }
arch/powerpc/kexec/file_load_64.c +0 -4
@@ -841,10 +841,6 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)vfree (image->arch.backup_buf); image->arch.backup_buf =NULL ; -vfree (image->elf_headers); - image->elf_headers =NULL ; - image->elf_headers_sz =0 ; -kvfree (image->arch.fdt); image->arch.fdt =NULL ;
arch/riscv/kernel/machine_kexec_file.c +0 -4
@@ -29,10 +29,6 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)kvfree (image->arch.fdt); image->arch.fdt =NULL ; -vfree (image->elf_headers); - image->elf_headers =NULL ; - image->elf_headers_sz =0 ; -return kexec_image_post_load_cleanup_default (image); }
arch/x86/kernel/machine_kexec_64.c +0 -4
@@ -600,10 +600,6 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,int arch_kimage_file_post_load_cleanup (struct kimage *image) { -vfree (image->elf_headers); - image->elf_headers =NULL ; - image->elf_headers_sz =0 ; -return kexec_image_post_load_cleanup_default (image); }#endif /* CONFIG_KEXEC_FILE */
kernel/kexec_file.c +9 -0
@@ -100,6 +100,13 @@ int kexec_image_post_load_cleanup_default(struct kimage *image)return image->fops->cleanup (image->image_loader_data); } +static void kexec_free_elf_headers (struct kimage *image) +{ +vfree (image->elf_headers); + image->elf_headers =NULL ; + image->elf_headers_sz =0 ; +} +/* * Free up memory used by kernel, initrd, and command line. This is temporary * memory allocation which is not needed any more after these buffers have
@@ -118,6 +125,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)kfree (image->cmdline_buf); image->cmdline_buf =NULL ; +kexec_free_elf_headers (image); +vfree (pi->purgatory_buf); pi->purgatory_buf =NULL ;
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 11/15] kexec: Free the elf core header in kimage_file_post_load_cleanup()
Sashiko has reviewed this patch and found no issues.
Quoted 5 lines
The elf core header buffer is generic state, but arm64, LoongArch, RISC-V, x86 and powerpc each free it in their post-load cleanup. Move it into the generic cleanup, which also gives the loader a helper to release it on error.
Quoted 16 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Huacai Chen <[email protected]>Cc Palmer Dabbelt <[email protected]>Cc Alexandre Ghiti <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc Borislav Petkov <[email protected]>Cc Dave Hansen <[email protected]>Cc Michael Ellerman <[email protected]>Cc Nicholas Piggin <[email protected]>Cc Christophe Leroy <[email protected]>Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
PATCH v7 12/15 kexec: Add kexec_load_other_segments() helper
In reply to Jinjie Ruanarm64, LoongArch and RISC-V duplicate the elf core header and initrd loading. Move it into a common helper with one arch hook for the boot parameters; a failure releases everything added for the attempt.
--- include/linux/kexec.h | 15 ++++++++ kernel/kexec_file.c | 90 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+)
include/linux/kexec.h +15 -0
@@ -259,6 +259,21 @@ int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len);extern int kexec_add_buffer (struct kexec_buf *kbuf);int kexec_locate_mem_hole (struct kexec_buf *kbuf); +struct kexec_load_params { +unsigned long kernel_load_addr; +unsigned long kernel_size; +char *initrd; +unsigned long initrd_len; +char *cmdline; +unsigned long cmdline_len; +}; + +int arch_kexec_setup_other_segments (struct kimage *image, +struct kexec_load_params *params, +unsigned long initrd_load_addr); +int kexec_load_other_segments (struct kimage *image, +struct kexec_load_params *params,bool top_down); +#ifndef arch_kexec_locate_mem_hole/** * arch_kexec_locate_mem_hole - Find free memory to place the segments.
kernel/kexec_file.c +90 -0
93 lines not on this page · raw message
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 12/15] kexec: Add kexec_load_other_segments() helper
Sashiko has reviewed this patch and found no issues.
Quoted 9 lines
arm64, LoongArch and RISC-V duplicate the elf core header and initrd loading. Move it into a common helper with one arch hook for the boot parameters; a failure releases everything added for the attempt.
Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Suggested-by Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
Mike Rapoport
maintainerRe: [PATCH v7 12/15] kexec: Add kexec_load_other_segments() helper
In reply to Jinjie RuanHi Jinjie,
Quoted 3 lines
arm64, LoongArch and RISC-V duplicate the elf core header and initrd loading. Move it into a common helper with one arch hook for the boot parameters; a failure releases everything added for the attempt.
I like the idea of pulling out the common code, but I'm not convinced that the split you suggest is right.
Quoted 16 lines
Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Suggested-by Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>--- include/linux/kexec.h | 15 ++++++++ kernel/kexec_file.c | 90 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+)include/linux/kexec.h +0
-0@@ -259,6 +259,21 @@ int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len);extern int kexec_add_buffer (struct kexec_buf *kbuf);
include/linux/kexec.h int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len); · line 269
Quoted 10 lines
include/linux/kexec.h +8
-0int kexec_locate_mem_hole (struct kexec_buf *kbuf); +struct kexec_load_params { +unsigned long kernel_load_addr; +unsigned long kernel_size; +char *initrd; +unsigned long initrd_len; +char *cmdline; +unsigned long cmdline_len; +};
initrd and cmdline are already in image->initrd_buf, initrd_buf_len, cmdline_buf and cmdline_buf_len, that's where fops->load() gets them from.
And the outputs are scattered: elf_load_addr goes to the image, the initrd address is passed as a separate argument to the hook, and LoongArch infers "there is an initrd" from initrd_load_addr != 0.
include/linux/kexec.h int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len); · line 276
Quoted 7 lines
include/linux/kexec.h +7
-0+ +int arch_kexec_setup_other_segments (struct kimage *image, +struct kexec_load_params *params, +unsigned long initrd_load_addr); +int kexec_load_other_segments (struct kimage *image, +struct kexec_load_params *params,bool top_down); +
The call chain is now fops->load() in arch -> kexec_load_other_segments() in core -> arch_kexec_setup_other_segments() back in arch. That's a
midlayer: the core helper owns the sequence and the arch fills a hole in the middle of it.
The weak default is never used, all three users override it, and x86, powerpc and s390 build a helper and a hook that nothing calls.
Quoted 18 lines
include/linux/kexec.h +0
-0#ifndef arch_kexec_locate_mem_hole/** * arch_kexec_locate_mem_hole - Find free memory to place the segments. kernel/kexec_file.c +7
-0@@ -1295,3 +1295,93 @@ int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, return 0; }#endif /* CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY */ + +int __weak arch_kexec_setup_other_segments (struct kimage *image, +struct kexec_load_params *params, +unsigned long initrd_load_addr) +{ +return 0 ; +}
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1314
Quoted 10 lines
kernel/kexec_file.c +10
-0+ +int kexec_load_other_segments (struct kimage *image, +struct kexec_load_params *params,bool top_down) +{ +struct kexec_buf kbuf = { + .image = image, +/* Do not allocate anything below the kernel */ + .buf_min = params->kernel_load_addr + params->kernel_size, + .buf_max = top_down ?PFN_PHYS (max_low_pfn) :ULONG_MAX , + };
top_down does not describe a direction, it selects "RISC-V policy" vs "arm64 policy": the upper limit for all buffers, the initrd direction and the 32G initrd window.
These are boot protocol constraints and they belong to the architecture. The 32G window comes from arm64 booting.rst and the SZ_64K alignment below is "largest supported page size" on arm64 and LoongArch. With this patch they become generic and RISC-V silently switches from ELF_CORE_HEADER_ALIGN to 64K in 15/15.
Quoted 5 lines
kernel/kexec_file.c +5
-0+ +unsigned long orig_segments = image->nr_segments; +unsigned long initrd_load_addr =0 ; +int ret =0 ; +
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1329
Quoted 10 lines
kernel/kexec_file.c +10
-0+#ifdef CONFIG_CRASH_DUMP +if (image->type ==KEXEC_TYPE_CRASH ) { + ret =crash_prepare_headers (true, &kbuf.buffer, &kbuf.bufsz,NULL ); +if (ret) { +pr_err ("Preparing elf core header failed\n" ); + goto out_err; + } + +if (unlikely (image->elf_headers)) +vfree (image->elf_headers);
The headers don't depend on where the kernel is placed, yet they are rebuilt on every iteration of the arm64 and LoongArch retry loops. That's why the vfree() above is needed, and with the out_err path freeing them it's now dead code.
Building the headers once and only placing them in the loop removes the leak instead of fixing it.
Quoted 45 lines
kernel/kexec_file.c +45
-0+ + image->elf_headers = kbuf.buffer; + image->elf_headers_sz = kbuf.bufsz; + + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; + kbuf.memsz = kbuf.bufsz; + kbuf.buf_align =SZ_64K ;/* largest supported page size */ + kbuf.top_down = true; + + ret =kexec_add_buffer (&kbuf); +if (ret) + goto out_err; + + image->elf_load_addr = kbuf.mem; + +kexec_dprintk ("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); + } +#endif + +if (params->initrd) { + kbuf.buffer = params->initrd; + kbuf.bufsz = params->initrd_len; + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; + kbuf.memsz = params->initrd_len; + kbuf.buf_align =0 ; +/* within 1GB-aligned window of up to 32GB in size */ +if (!top_down) + kbuf.buf_max =round_down (params->kernel_load_addr,SZ_1G ) + + (unsigned long )SZ_1G *32 ; + kbuf.top_down = top_down; + + ret =kexec_add_buffer (&kbuf); +if (ret) + goto out_err; + initrd_load_addr = kbuf.mem; + +kexec_dprintk ("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , + initrd_load_addr, kbuf.bufsz, kbuf.memsz); + } + + ret =arch_kexec_setup_other_segments (image, params, + initrd_load_addr); +if (ret) + goto out_err;
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1384
Quoted 10 lines
kernel/kexec_file.c +10
-0+ +return 0 ; + +out_err: +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--; + } + +kexec_free_elf_headers (image);
This drops segments added by the arch hook (DTB, RISC-V purgatory, arm64 dm-crypt keys) without knowing anything about the state attached to them, like image->dm_crypt_keys_addr or the purgatory info. The hook, in turn, frees its buffers but not its segments.
Nobody owns the unwind, so "a failure releases everything added for the attempt" is not quite what happens.
The conversions also bury ordering changes in "simplify" patches: on arm64 dm-crypt keys move after the initrd, and on RISC-V the initrd moves before the purgatory. And dm-crypt keys loading is not an arm64 boot parameter, x86 and powerpc use it as well.
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1387
Quoted 3 lines
kernel/kexec_file.c +3
-0+ +return ret; +}
What I had in mind was to make the core provide building blocks and let the architectures compose them, e.g. a common rollback function that frees CMAs and drops segments above nr. The retry loops in arm64 and LoongArch image loaders can use it instead of open coding kexec_free_segment_cma() and nr_segments--. There also could be a helper for adding FDT for example and maybe some other common parts, like, say, efl headers.
Arch's load_other_segments() could use these helpers and still define their own policy for segment placement.
Signature · 2 lines
Sincerely yours, Mike.
Re: [PATCH v7 12/15] kexec: Add kexec_load_other_segments() helper
Quoted 1 line · Mike Rapoport
Hi Jinjie,
Hi Mike,
Quoted 6 lines
Quoted 4 lines
arm64, LoongArch and RISC-V duplicate the elf core header and initrd loading. Move it into a common helper with one arch hook for the boot parameters; a failure releases everything added for the attempt.
I like the idea of pulling out the common code, but I'm not convinced
Thank you for the review.
Quoted 1 line
that the split you suggest is right.
Yes, the current solution is straightforward.
Quoted 19 lines
Quoted 19 lines
Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Suggested-by Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>--- include/linux/kexec.h | 15 ++++++++ kernel/kexec_file.c | 90 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+)include/linux/kexec.h +0
-0@@ -259,6 +259,21 @@ int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len);extern int kexec_add_buffer (struct kexec_buf *kbuf);int kexec_locate_mem_hole (struct kexec_buf *kbuf);
Quoted 11 lines
include/linux/kexec.h int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len); · line 269
Quoted 8 lines
include/linux/kexec.h +8
-0+struct kexec_load_params { +unsigned long kernel_load_addr; +unsigned long kernel_size; +char *initrd; +unsigned long initrd_len; +char *cmdline; +unsigned long cmdline_len; +};initrd and cmdline are already in
image->initrd_buf,initrd_buf_len,cmdline_bufandcmdline_buf_len, that's wherefops->load()gets them from.
OK, will delete them.
Quoted 4 lines
And the outputs are scattered:
elf_load_addrgoes to the image, the initrd address is passed as a separate argument to the hook, and LoongArch infers "there is an initrd" frominitrd_load_addr!= 0.
These are currently just being shoehorned in.
Quoted 15 lines
include/linux/kexec.h int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len); · line 276
Quoted 10 lines
include/linux/kexec.h +7
-0+ +int arch_kexec_setup_other_segments (struct kimage *image, +struct kexec_load_params *params, +unsigned long initrd_load_addr); +int kexec_load_other_segments (struct kimage *image, +struct kexec_load_params *params,bool top_down); +The call chain is now
fops->load()in arch ->kexec_load_other_segments()in core ->arch_kexec_setup_other_segments()back in arch. That's a
midlayer: the core helper owns the sequence and the arch fills a hole in the middle of it.
The new refactoring will follow this principle to ensure that the framework code is in control of the order.
Quoted 3 lines
The weak default is never used, all three users override it, and x86, powerpc and s390 build a helper and a hook that nothing calls.
Yes, x86 and powerpc has not used it, Some adjustments need for the rework.
Quoted 25 lines
Quoted 25 lines
include/linux/kexec.h +0
-0#ifndef arch_kexec_locate_mem_hole/** * arch_kexec_locate_mem_hole - Find free memory to place the segments. kernel/kexec_file.c +13
-0@@ -1295,3 +1295,93 @@ int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,return 0 ; }#endif /* CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY */ + +int __weak arch_kexec_setup_other_segments (struct kimage *image, +struct kexec_load_params *params, +unsigned long initrd_load_addr) +{ +return 0 ; +} + +int kexec_load_other_segments (struct kimage *image, +struct kexec_load_params *params,bool top_down) +{ +struct kexec_buf kbuf = { + .image = image,
Quoted 8 lines
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1314
Quoted 4 lines
kernel/kexec_file.c +4
-0+/* Do not allocate anything below the kernel */ + .buf_min = params->kernel_load_addr + params->kernel_size, + .buf_max = top_down ? PFN_PHYS(max_low_pfn) : ULONG_MAX, + };
top_downdoes not describe a direction, it selects "RISC-V policy" vs "arm64 policy": the upper limit for all buffers, the initrd direction and the 32G initrd window.
Yes its the topdown of initrd and distinguish between different architectures is a bit of a stretch here.
Right! arm64 has its unique boot protocol restrictions. Here, RISC-V has a bug fix that requires specifying a memory top limit.
Quoted 2 lines
These are boot protocol constraints and they belong to the architecture.
Agree
Quoted 2 lines
The 32G window comes from arm64
booting.rstand theSZ_64Kalignment below is "largest supported page size" on arm64 and LoongArch. With this patch
Yes.
Quoted 2 lines
they become generic and RISC-V silently switches from
ELF_CORE_HEADER_ALIGNto 64K in 15/15.
There are indeed some function changes, which are also mentioned in the commit message.
Quoted 14 lines
Quoted 14 lines
kernel/kexec_file.c +12
-0+ +unsigned long orig_segments = image->nr_segments; +unsigned long initrd_load_addr =0 ; +int ret =0 ; + +#ifdef CONFIG_CRASH_DUMP +if (image->type ==KEXEC_TYPE_CRASH ) { + ret =crash_prepare_headers (true, &kbuf.buffer, &kbuf.bufsz,NULL ); +if (ret) { +pr_err ("Preparing elf core header failed\n" ); + goto out_err; + }
Quoted 8 lines
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1329
Quoted 3 lines
kernel/kexec_file.c +3
-0+ +if (unlikely (image->elf_headers)) +vfree (image->elf_headers);The headers don't depend on where the kernel is placed, yet they are rebuilt on every iteration of the arm64 and LoongArch retry loops. That's why the
vfree()above is needed, and with theout_errpath freeing them it's now dead code.
Yes, it need deleted now.
Quoted 3 lines
Building the headers once and only placing them in the loop removes the leak instead of fixing it.
I agree, I had a similar fix attempt before, where I only allocated once to avoid placing it in a loop.
Quoted 54 lines
Quoted 54 lines
kernel/kexec_file.c +52
-0+ + image->elf_headers = kbuf.buffer; + image->elf_headers_sz = kbuf.bufsz; + + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; + kbuf.memsz = kbuf.bufsz; + kbuf.buf_align =SZ_64K ;/* largest supported page size */ + kbuf.top_down = true; + + ret =kexec_add_buffer (&kbuf); +if (ret) + goto out_err; + + image->elf_load_addr = kbuf.mem; + +kexec_dprintk ("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); + } +#endif + +if (params->initrd) { + kbuf.buffer = params->initrd; + kbuf.bufsz = params->initrd_len; + kbuf.mem =KEXEC_BUF_MEM_UNKNOWN ; + kbuf.memsz = params->initrd_len; + kbuf.buf_align =0 ; +/* within 1GB-aligned window of up to 32GB in size */ +if (!top_down) + kbuf.buf_max =round_down (params->kernel_load_addr,SZ_1G ) + + (unsigned long )SZ_1G *32 ; + kbuf.top_down = top_down; + + ret =kexec_add_buffer (&kbuf); +if (ret) + goto out_err; + initrd_load_addr = kbuf.mem; + +kexec_dprintk ("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , + initrd_load_addr, kbuf.bufsz, kbuf.memsz); + } + + ret =arch_kexec_setup_other_segments (image, params, + initrd_load_addr); +if (ret) + goto out_err; + +return 0 ; + +out_err: +while (image->nr_segments > orig_segments) { +kexec_free_segment_cma (image, image->nr_segments -1 ); + image->nr_segments--;
Quoted 8 lines
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1384
Quoted 3 lines
kernel/kexec_file.c +3
-0+ } + +kexec_free_elf_headers (image);This drops segments added by the arch hook (DTB, RISC-V purgatory, arm64 dm-crypt keys) without knowing anything about the state attached to them, like
image->dm_crypt_keys_addror the purgatory info. The hook, in turn, frees its buffers but not its segments.
Yes
Quoted 9 lines
Nobody owns the unwind, so "a failure releases everything added for the attempt" is not quite what happens.
The conversions also bury ordering changes in "simplify" patches: on arm64 dm-crypt keys move after the initrd, and on RISC-V the initrd moves before the purgatory. And dm-crypt keys loading is not an arm64 boot parameter, x86 and powerpc use it as well.
Yes, the segment allocation order is indeed changed.
Quoted 10 lines
kernel/kexec_file.c int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name, · line 1387
Quoted 3 lines
kernel/kexec_file.c +3
-0+ +return ret; +}What I had in mind was to make the core provide building blocks and let the architectures compose them, e.g. a common rollback function that frees CMAs and drops segments above nr. The retry loops in arm64 and LoongArch image loaders can use it instead of open coding
kexec_free_segment_cma()and nr_segments--. There also could be a helper
I agree.
Quoted 2 lines
for adding FDT for example and maybe some other common parts, like, say, efl headers.
I am trying out your refactoring ideas so that x86 and PowerPC can also use these newly added small hook functions.
Quoted 3 lines
Arch's
load_other_segments()could use these helpers and still define their own policy for segment placement.
This will eliminate the issue of changes in the above sequence or the problem of the startup protocol being modified.
Thank you again for your review and suggestions for refactoring. I will thoroughly consider the issues you raised, revise the code, and conduct more testing.
Quoted 1 line
Signature · 2 lines
Best regards, Jinjie
PATCH v7 13/15 arm64: kexec_file: Use kexec_load_other_segments() to simplify code
In reply to Jinjie RuanReplace the local copy with the common helper.
The retry loop stays. And the DTB and the dm-crypt keys move to the setup hook and the keys are loaded after the initrd now.
--- arch/arm64/include/asm/kexec.h | 5 -- arch/arm64/kernel/kexec_image.c | 12 +++- arch/arm64/kernel/machine_kexec_file.c | 95 ++++---------------------- 3 files changed, 23 insertions(+), 89 deletions(-)
arch/arm64/include/asm/kexec.h +0 -5
@@ -123,11 +123,6 @@ extern const struct kexec_file_ops kexec_image_ops;int arch_kimage_file_post_load_cleanup (struct kimage *image);#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup - -extern int load_other_segments (struct kimage *image, -unsigned long kernel_load_addr,unsigned long kernel_size, -char *initrd,unsigned long initrd_len, -char *cmdline);#endif #endif /* __ASSEMBLER__ */
arch/arm64/kernel/kexec_image.c +9 -3
@@ -44,6 +44,12 @@ static void *image_load(struct kimage *image,struct kexec_buf kbuf = {};unsigned long text_offset, kernel_segment_number;struct kexec_segment *kernel_segment; +struct kexec_load_params params = { + .initrd = initrd, + .initrd_len = initrd_len, + .cmdline = cmdline, + .cmdline_len = cmdline_len, + };int ret;/*
@@ -101,9 +107,9 @@ static void *image_load(struct kimage *image,while ((ret =kexec_add_buffer (&kbuf)) ==0 ) {/* Try to load additional data */ kernel_segment = &image->segment[kernel_segment_number]; - ret =load_other_segments (image, kernel_segment->mem, - kernel_segment->memsz, initrd, - initrd_len, cmdline); + params.kernel_load_addr = kernel_segment->mem; + params.kernel_size = kernel_segment->memsz; + ret =kexec_load_other_segments (image, ¶ms, false);if (!ret)break ;
arch/arm64/kernel/machine_kexec_file.c +14 -81
125 lines not on this page · raw message
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 13/15] arm64: kexec_file: Use kexec_load_other_segments() to simplify code
Sashiko has reviewed this patch and found no issues.
Quoted 11 lines
Replace the local copy with the common helper.
The retry loop stays. And the DTB and the dm-crypt keys move to the setup hook and the keys are loaded after the initrd now.
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
PATCH v7 14/15 LoongArch: kexec_file: Use kexec_load_other_segments() to simplify code
In reply to Jinjie RuanReplace the local copy with the common helper. The retry loop stays and the command line is built in the setup hook.
--- arch/loongarch/include/asm/kexec.h | 4 - arch/loongarch/kernel/kexec_efi.c | 12 ++- arch/loongarch/kernel/kexec_elf.c | 11 ++- arch/loongarch/kernel/machine_kexec_file.c | 94 ++++------------------ 4 files changed, 32 insertions(+), 89 deletions(-)
arch/loongarch/include/asm/kexec.h +0 -4
@@ -49,10 +49,6 @@ extern const struct kexec_file_ops kexec_elf_ops;int arch_kimage_file_post_load_cleanup (struct kimage *image);#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup - -extern int load_other_segments (struct kimage *image, -unsigned long kernel_load_addr,unsigned long kernel_size, -char *initrd,unsigned long initrd_len,char *cmdline,unsigned long cmdline_len);#endif typedef void (*do_kexec_t)(unsigned long efi_boot,
arch/loongarch/kernel/kexec_efi.c +9 -3
@@ -44,6 +44,12 @@ static void *efi_kexec_load(struct kimage *image,unsigned long text_offset, kernel_segment_number;struct kexec_buf kbuf = {};struct kexec_segment *kernel_segment; +struct kexec_load_params params = { + .initrd = initrd, + .initrd_len = initrd_len, + .cmdline = cmdline, + .cmdline_len = cmdline_len, + };struct loongarch_image_header *h; h = (struct loongarch_image_header *)kernel;
@@ -76,9 +82,9 @@ static void *efi_kexec_load(struct kimage *image,while ((ret =kexec_add_buffer (&kbuf)) ==0 ) {/* Try to load additional data */ kernel_segment = &image->segment[kernel_segment_number]; - ret =load_other_segments (image, kernel_segment->mem, - kernel_segment->memsz, initrd, - initrd_len, cmdline, cmdline_len); + params.kernel_load_addr = kernel_segment->mem; + params.kernel_size = kernel_segment->memsz; + ret =kexec_load_other_segments (image, ¶ms, false);if (!ret)break ;
arch/loongarch/kernel/kexec_elf.c +9 -2
@@ -62,6 +62,12 @@ static void *elf_kexec_load(struct kimage *image,struct kexec_buf kbuf = {};struct kexec_elf_info elf_info;struct kexec_segment *kernel_segment; +struct kexec_load_params params = { + .initrd = initrd, + .initrd_len = initrd_len, + .cmdline = cmdline, + .cmdline_len = cmdline_len, + }; ret =kexec_build_elf_info (kernel, kernel_len, &ehdr, &elf_info);if (ret <0 )
@@ -83,8 +89,9 @@ static void *elf_kexec_load(struct kimage *image,/* Load additional data */ kernel_segment = &image->segment[kernel_segment_number]; - ret =load_other_segments (image, kernel_segment->mem, kernel_segment->memsz, - initrd, initrd_len, cmdline, cmdline_len); + params.kernel_load_addr = kernel_segment->mem; + params.kernel_size = kernel_segment->memsz; + ret =kexec_load_other_segments (image, ¶ms, false);if (ret <0 ) goto out;
arch/loongarch/kernel/machine_kexec_file.c +14 -80
126 lines not on this page · raw message
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 14/15] LoongArch: kexec_file: Use kexec_load_other_segments() to simplify code
Sashiko has reviewed this patch and found no issues.
Quoted 8 lines
Replace the local copy with the common helper. The retry loop stays and the command line is built in the setup hook.
Cc Huacai Chen <[email protected]>Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
PATCH v7 15/15 riscv: kexec_file: Use kexec_load_other_segments() to simplify code
In reply to Jinjie RuanReplace load_extra_segments() with the common helper.
The purgatory, DTB and kdump command line move to the setup hook. And the elf core header is now 64K aligned.
--- arch/riscv/include/asm/kexec.h | 4 -- arch/riscv/kernel/kexec_elf.c | 11 ++++- arch/riscv/kernel/kexec_image.c | 11 ++++- arch/riscv/kernel/machine_kexec_file.c | 65 ++++++-------------------- 4 files changed, 31 insertions(+), 60 deletions(-)
arch/riscv/include/asm/kexec.h +0 -4
@@ -69,10 +69,6 @@ struct kimage;int arch_kimage_file_post_load_cleanup (struct kimage *image);#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup -int load_extra_segments (struct kimage *image,unsigned long kernel_start, -unsigned long kernel_len,char *initrd, -unsigned long initrd_len,char *cmdline, -unsigned long cmdline_len);#endif #endif
arch/riscv/kernel/kexec_elf.c +9 -2
@@ -110,6 +110,12 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,unsigned long new_kernel_pbase =0UL ;struct elfhdr ehdr;struct kexec_elf_info elf_info; +struct kexec_load_params params = { + .initrd = initrd, + .initrd_len = initrd_len, + .cmdline = cmdline, + .cmdline_len = cmdline_len, + }; ret =kexec_build_elf_info (kernel_buf, kernel_len, &ehdr, &elf_info);if (ret)
@@ -126,8 +132,9 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,if (ret) goto out; - ret =load_extra_segments (image, image->start, kernel_len, - initrd, initrd_len, cmdline, cmdline_len); + params.kernel_load_addr = image->start; + params.kernel_size = kernel_len; + ret =kexec_load_other_segments (image, ¶ms, true); out:kexec_free_elf_info (&elf_info);return ret ?ERR_PTR (ret) :NULL ;
arch/riscv/kernel/kexec_image.c +9 -2
@@ -42,6 +42,12 @@ static void *image_load(struct kimage *image,u64 flags;bool be_image, be_kernel;struct kexec_buf kbuf = {}; +struct kexec_load_params params = { + .initrd = initrd, + .initrd_len = initrd_len, + .cmdline = cmdline, + .cmdline_len = cmdline_len, + };int ret;/* Check Image header */
@@ -83,8 +89,9 @@ static void *image_load(struct kimage *image,pr_info ("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n" , kbuf.mem, kbuf.bufsz, kbuf.memsz); - ret =load_extra_segments (image, kbuf.mem, kbuf.memsz, - initrd, initrd_len, cmdline, cmdline_len); + params.kernel_load_addr = kbuf.mem; + params.kernel_size = kbuf.memsz; + ret =kexec_load_other_segments (image, ¶ms, true); out:return ret ?ERR_PTR (ret) :NULL ;
arch/riscv/kernel/machine_kexec_file.c +13 -52
87 lines not on this page · raw message
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v7 15/15] riscv: kexec_file: Use kexec_load_other_segments() to simplify code
Sashiko has reviewed this patch and found no issues.
Quoted 11 lines
Replace
load_extra_segments()with the common helper.The purgatory, DTB and kdump command line move to the setup hook. And the elf core header is now 64K aligned.
Cc Palmer Dabbelt <[email protected]>Cc Alexandre Ghiti <[email protected]>Cc Andrew Morton <[email protected]>Cc Baoquan He <[email protected]>Cc Mike Rapoport <[email protected]>Signed-off-by Jinjie Ruan <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
Quoted 13 lines · Jinjie Ruan
ARM64, LoongArch and RISC-V load the elf core header, initrd, DTB and cmdline parameters with almost identical code. This series fixes the bugs in that path. And as Mike suggested, rework the common part into a helper.
Patch 1 stops using the Image
text_offsetfor arm64, which breaks CMA segments in both the load and release paths, and rejects images with a non-zero value.Patches 2-7 fix the CMA page and elf core header leaks in the retry loops in arm64 and LoongArch, also fix a
modified_cmdlineleak in LoongArch.
Quoted 8 lines
Patches 8-10 pass the kbuf fields to
crash_prepare_headers()to simplfy code.Patch 11 frees the elf core header from the generic post-load cleanup.
Patches 12-15 add
kexec_load_other_segments()and convert the three architectures, andarch_kexec_setup_other_segments()builds the architecture specific boot parameters (a DTB or a command line).
Hi, everyone,
Any thoughts or suggestions?
Quoted 62 lines
Tested with
kexec_file_load()and kdump on arm64, RISC-V and LoongArch under QEMU, including the non-zerotext_offsetrejection.This series is also available at:
https://github.com/ruanjinjie-eng/linux/tree/kexec_cleanup_v7Changes in v7:
- Rework thekexec_fileload code to avoid code duplication
as Mike suggested.
- Add Acked-by.
- Drop the two non-zerotext_offsetfix patches
and deprecate .text_offset in arm64 as Mike suggested.
- Drop the x86-32 fix patch.
- Drop the TOCTOU race fix patches.
- Link to v6: https://lore.kernel.org/all/[email protected]/Previous Versions are as below:
- v5: https://lore.kernel.org/all/[email protected]/
- v4: https://lore.kernel.org/all/[email protected]/
- v3: https://lore.kernel.org/all/[email protected]/
- v2: https://lore.kernel.org/all/[email protected]/
- v1: https://lore.kernel.org/all/[email protected]/#tJinjie Ruan (15):
arm64:kexec_file: Stop using the Imagetext_offset
kexec: Extractkexec_free_segment_cma()fromkimage_free_cma()
arm64:kexec_file: Fix CMA page leaks in segment placement retry loops
arm64:kexec_file: Fixelf_headersmemory leak in retry loop
LoongArch:kexec_file: Fix CMA page leaks in segment placement retry
loops
LoongArch:kexec_file: Fixelf_headersmemory leak in retry loop
LoongArch:kexec_file: Fix amodified_cmdlineleak
arm64:kexec_file: Simplifyload_other_segments()
LoongArch:kexec_file: Simplifyload_other_segments()
riscv:kexec_file: Simplifyload_extra_segments()
kexec: Free the elf core header inkimage_file_post_load_cleanup()
kexec: Addkexec_load_other_segments()helper
arm64:kexec_file: Usekexec_load_other_segments()to simplify code
LoongArch:kexec_file: Usekexec_load_other_segments()to simplify
code
riscv:kexec_file: Usekexec_load_other_segments()to simplify codearch/arm64/include/asm/kexec.h | 5 - arch/arm64/kernel/kexec_image.c | 33 +++++-- arch/arm64/kernel/machine_kexec_file.c | 103 ++++----------------- arch/loongarch/include/asm/kexec.h | 4 - arch/loongarch/kernel/kexec_efi.c | 20 +++- arch/loongarch/kernel/kexec_elf.c | 11 ++- arch/loongarch/kernel/machine_kexec.c | 2 + arch/loongarch/kernel/machine_kexec_file.c | 96 +++---------------- arch/powerpc/kexec/file_load_64.c | 4 - arch/riscv/include/asm/kexec.h | 4 - arch/riscv/kernel/kexec_elf.c | 11 ++- arch/riscv/kernel/kexec_image.c | 11 ++- arch/riscv/kernel/machine_kexec_file.c | 79 ++++------------ arch/x86/kernel/machine_kexec_64.c | 4 - include/linux/kexec.h | 17 ++++ kernel/kexec_core.c | 25 ++--- kernel/kexec_file.c | 99 ++++++++++++++++++++ 17 files changed, 248 insertions(+), 280 deletions(-)
Signature · 2 lines
Best regards, Jinjie
Mike Rapoport
maintainerHi Jinjie,
Quoted 5 lines
ARM64, LoongArch and RISC-V load the elf core header, initrd, DTB and cmdline parameters with almost identical code. This series fixes the bugs in that path. And as Mike suggested, rework the common part into a helper.
Quoted 7 lines
Patch 1 stops using the Image
text_offsetfor arm64, which breaks CMA segments in both the load and release paths, and rejects images with a non-zero value.Patches 2-7 fix the CMA page and elf core header leaks in the retry loops in arm64 and LoongArch, also fix a
modified_cmdlineleak in LoongArch.
These mix fixes that can be easily merged via arch trees and something that depends on a change to the generic kexec code.
Besides, elf_headers leak and arm64's text_offset fixes should be applied to most if not all stable kernels, while CMA fixes are only for the relatively new kernels.
I'd suggest splitting
arm64: kexec_file: Stop using the Image text_offset
arm64: kexec_file: Fix elf_headers memory leak in retry loop
and sending as pure arm64 material, and
LoongArch: kexec_file: Fix elf_headers memory leak in retry loop
LoongArch: kexec_file: Fix a modified_cmdline leak
as pure LoongArch material.
And then working on CMA fixes after these land.
If there are real dependencies or potential conflicts, b4 prep can help you with managing them.
Quoted 8 lines
Patches 8-10 pass the kbuf fields to
crash_prepare_headers()to simplfy code.Patch 11 frees the elf core header from the generic post-load cleanup.
Patches 12-15 add
kexec_load_other_segments()and convert the three architectures, andarch_kexec_setup_other_segments()builds the architecture specific boot parameters (a DTB or a command line).
I think the refactoring should go before the identical fixes in arch/ code.
Signature · 2 lines
Sincerely yours, Mike.
Quoted 2 lines · Mike Rapoport
Hi Jinjie,
Hi Mike,
Quoted 9 lines
Quoted 9 lines
ARM64, LoongArch and RISC-V load the elf core header, initrd, DTB and cmdline parameters with almost identical code. This series fixes the bugs in that path. And as Mike suggested, rework the common part into a helper.
Patch 1 stops using the Image
text_offsetfor arm64, which breaks CMA segments in both the load and release paths, and rejects images with a non-zero value.
Quoted 6 lines
Quoted 3 lines
Patches 2-7 fix the CMA page and elf core header leaks in the retry loops in arm64 and LoongArch, also fix a
modified_cmdlineleak in LoongArch.These mix fixes that can be easily merged via arch trees and something that depends on a change to the generic kexec code.
Thank you! I will split them to send in v8.
Quoted 4 lines
Besides,
elf_headersleak and arm64'stext_offsetfixes should be applied to most if not all stable kernels, while CMA fixes are only for the relatively new kernels.
You are right. arm64 text_offset fix involves many versions.
If I remember correctly, the CMA patch that was fixed was only merged this year.
Quoted 9 lines
I'd suggest splitting
arm64:
kexec_file: Stop using the Imagetext_offset
arm64:kexec_file: Fixelf_headersmemory leak in retry loopand sending as pure arm64 material, and
Quoted 6 lines
LoongArch:
kexec_file: Fixelf_headersmemory leak in retry loop
LoongArch:kexec_file: Fix amodified_cmdlineleakas pure LoongArch material.
And then working on CMA fixes after these land.
I agree with doing this; the patch will be cleaner and won't mix different architectures with common code.
Quoted 3 lines
If there are real dependencies or potential conflicts, b4 prep can help you with managing them.
Indeed.
Quoted 4 lines
Quoted 4 lines
Patches 8-10 pass the kbuf fields to
crash_prepare_headers()to simplfy code.
Quoted 8 lines
Quoted 5 lines
Patch 11 frees the elf core header from the generic post-load cleanup.
Patches 12-15 add
kexec_load_other_segments()and convert the three architectures, andarch_kexec_setup_other_segments()builds the architecture specific boot parameters (a DTB or a command line).I think the refactoring should go before the identical fixes in
arch/code.
However, this will cause trouble for the fix patch in the stable round.
Quoted 1 line
Signature · 2 lines
Best regards, Jinjie
Mike Rapoport
maintainerQuoted 7 lines · Jinjie Ruan
Quoted 5 lines · Mike Rapoport
Quoted 5 lines
Patches 8-10 pass the kbuf fields to
crash_prepare_headers()to simplfy code.Patch 11 frees the elf core header from the generic post-load cleanup.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Patches 12-15 add
kexec_load_other_segments()and convert the three architectures, andarch_kexec_setup_other_segments()builds the architecture specific boot parameters (a DTB or a command line).I think the refactoring should go before the identical fixes in
arch/code.However, this will cause trouble for the fix patch in the stable round.
That's right, but since actual fixes anyway require changes to kexec core, I think it's better to refactor first. The core than will take care of proper freeing in the most cases.
And since CMA support in kexec is quite new I think the entire set can be backported to stable then.
I'm planning to have a closer look at the refactoring Really Soon :)
Quoted 4 lines
Signature · 2 lines
Best regards, Jinjie
Signature · 2 lines
Sincerely yours, Mike.
Quoted 12 lines · Mike Rapoport
Quoted 11 lines · Jinjie Ruan
Quoted 9 lines · Mike Rapoport
Quoted 8 lines
Patches 8-10 pass the kbuf fields to
crash_prepare_headers()to simplfy code.Patch 11 frees the elf core header from the generic post-load cleanup.
Patches 12-15 add
kexec_load_other_segments()and convert the three architectures, andarch_kexec_setup_other_segments()builds the architecture specific boot parameters (a DTB or a command line).
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I think the refactoring should go before the identical fixes in
arch/code.However, this will cause trouble for the fix patch in the stable round.
That's right, but since actual fixes anyway require changes to kexec core, I think it's better to refactor first. The core than will take care of proper freeing in the most cases.
This way, each architecture doesn't have to worry about the issue of incomplete resource release.
Quoted 3 lines
And since CMA support in kexec is quite new I think the entire set can be backported to stable then.
Make sense for me.
Quoted 2 lines
I'm planning to have a closer look at the refactoring Really Soon :)
Thank you very much! Looking forward to your review.
Quoted 6 lines
Quoted 5 lines
Signature · 3 lines
-- Best regards, Jinjie
Signature · 2 lines
Best regards, Jinjie