Re: [PATCH v3 01/17] kexec: Record allocated CMA pages to fix release size mismatch

Hi,

> The CMA pages allocated for a kexec segment are released using the
> segment's memsz to calculate the number of pages. However, some
> architecture loaders modify the segment's memsz after allocation
> (e.g. arm64 subtracts text_offset), causing the release function to
> free fewer pages than were originally allocated, leaking the remaining
> CMA pages.
> 
> Add a per-segment `segment_cma_pages` array to store the number of
> pages actually allocated from CMA. Populate it during
> kexec_add_buffer() using the aligned memsz, and use it in
> kimage_free_cma() to accurately release all allocated pages.
> 
> This avoids relying on the potentially modified segment->memsz and
> prevents silent CMA memory leaks.
> 
> 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: Brian Mak <[email protected]>
> Cc: Pingfan Liu <[email protected]>
> Cc: Sourabh Jain <[email protected]>
> Cc: Justinien Bouron <[email protected]>
> Cc: Li Chen <[email protected]>
> Cc: [email protected]
> Link: https://sashiko.dev/#/patchset/20260729031235.2840255-1-ruanjinjie%40huawei.com
> Fixes: 07d24902977e ("kexec: enable CMA based contiguous allocation")
> Signed-off-by: Jinjie Ruan <[email protected]>
>
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 0af8ae4fdd087..83c296c0eb6cc 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -349,6 +349,7 @@ struct kimage {
>  	unsigned long nr_segments;
>  	struct kexec_segment segment[KEXEC_SEGMENT_MAX];
>  	struct page *segment_cma[KEXEC_SEGMENT_MAX];
> +	unsigned int segment_cma_pages[KEXEC_SEGMENT_MAX];

Can we universally use unsigned long for number of pages?

>  
>  	struct list_head control_pages;
>  	struct list_head dest_pages;
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index dc770b9a6d053..611b15bb1369e 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -560,7 +560,7 @@ static void kimage_free_cma(struct kimage *image)
>  
>  	for (i = 0; i < image->nr_segments; i++) {
>  		struct page *cma = image->segment_cma[i];
> -		u32 nr_pages = image->segment[i].memsz >> PAGE_SHIFT;
> +		unsigned int nr_pages = image->segment_cma_pages[i];
>  
>  		if (!cma)
>  			continue;
> @@ -568,6 +568,7 @@ static void kimage_free_cma(struct kimage *image)
>  		arch_kexec_pre_free_pages(page_address(cma), nr_pages);
>  		dma_release_from_contiguous(NULL, cma, nr_pages);
>  		image->segment_cma[i] = NULL;
> +		image->segment_cma_pages[i] = 0;
>  	}
>  
>  }
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 59fb9d71e9d86..bfae3fee7f2f9 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -670,7 +670,7 @@ static int kexec_walk_resources(struct kexec_buf *kbuf,
>  
>  static int kexec_alloc_contig(struct kexec_buf *kbuf)
>  {
> -	size_t nr_pages = kbuf->memsz >> PAGE_SHIFT;
> +	size_t nr_pages = PFN_DOWN(kbuf->memsz);
>  	unsigned long mem;
>  	struct page *p;
>  
> @@ -756,6 +756,8 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
>   */
>  int kexec_add_buffer(struct kexec_buf *kbuf)
>  {
> +	unsigned long nr_segments = kbuf->image->nr_segments;
> +	size_t nr_pages;
>  	struct kexec_segment *ksegment;
>  	int ret;
>  
> @@ -763,7 +765,7 @@ int kexec_add_buffer(struct kexec_buf *kbuf)
>  	if (!kbuf->image->file_mode)
>  		return -EINVAL;
>  
> -	if (kbuf->image->nr_segments >= KEXEC_SEGMENT_MAX)
> +	if (nr_segments >= KEXEC_SEGMENT_MAX)
>  		return -EINVAL;
>  
>  	/*
> @@ -789,12 +791,18 @@ int kexec_add_buffer(struct kexec_buf *kbuf)
>  		return ret;
>  
>  	/* Found a suitable memory range */
> -	ksegment = &kbuf->image->segment[kbuf->image->nr_segments];
> +	ksegment = &kbuf->image->segment[nr_segments];
>  	ksegment->kbuf = kbuf->buffer;
>  	ksegment->bufsz = kbuf->bufsz;
>  	ksegment->mem = kbuf->mem;
>  	ksegment->memsz = kbuf->memsz;
> -	kbuf->image->segment_cma[kbuf->image->nr_segments] = kbuf->cma;
> +	kbuf->image->segment_cma[nr_segments] = kbuf->cma;
> +	if (kbuf->cma) {
> +		nr_pages = (unsigned int)(PFN_DOWN(kbuf->memsz));
> +		kbuf->image->segment_cma_pages[nr_segments] = nr_pages;
		kbuf->image->segment_cma_pages[nr_segments] = nr_pages;
> +	} else {
> +		kbuf->image->segment_cma_pages[nr_segments] = 0;
> +	}

I suggest to rename nr_pages to nr_cma_pages, initialize it to 0 at
declaration time and make this

	if (kbuf->cma)
		nr_cma_pages = PFN_DOWN(kbuf->memsz);
	kbuf->image->segment_cma_pages[nr_segments] = nr_cma_pages;

-- 
Sincerely yours,
Mike.