Received-SPF: pass (google.com: domain of linux-kernel+bounces-87977-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 1 Mar 2024 14:52:34 +0800
From: Baoquan He <bhe@redhat.com>
To: "yang.zhang" <gaoshanliukou@163.com>, akpm@linux-foundation.org
Cc: ebiederm@xmission.com, linux-kernel@vger.kernel.org,
	kexec@lists.infradead.org, "yang.zhang" <yang.zhang@hexintek.com>
Subject: Re: [PATCH V3] kexec: copy only happens before uchunk goes to zero
Message-ID: <ZeF7Muc8WyuQ1bPR@MiWiFi-R3L-srv>
References: <20240222092119.5602-1-gaoshanliukou@163.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240222092119.5602-1-gaoshanliukou@163.com>

CC Andrew

On 02/22/24 at 05:21pm, yang.zhang wrote:
> From: "yang.zhang" <yang.zhang@hexintek.com>
> 
> When loading segments, ubytes is <= mbytes. When ubytes is exhausted,
> there could be remaining mbytes. Then in the while loop, the buf pointer
> advancing with mchunk will causing meaningless reading even though it
> doesn't harm.
> 
> So let's change to make sure that all of the copying and the rest only
> happens before uchunk goes to zero.
> 
> Acked-by: Baoquan He <bhe@redhat.com>
> Signed-off-by: yang.zhang <yang.zhang@hexintek.com>
> 
> ---
> V2 -> V3:
> - Add more detailed description in the commit message
> v1 -> v2:
> - Only copy before uchunk goes to zero
> 
> V1: https://lore.kernel.org/lkml/20240130101802.23850-1-gaoshanliukou@163.com/
> ---
>  kernel/kexec_core.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index d08fc7b5db97..2fc3d0e3715a 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -800,22 +800,24 @@ static int kimage_load_normal_segment(struct kimage *image,
>  				PAGE_SIZE - (maddr & ~PAGE_MASK));
>  		uchunk = min(ubytes, mchunk);
>  
> -		/* For file based kexec, source pages are in kernel memory */
> -		if (image->file_mode)
> -			memcpy(ptr, kbuf, uchunk);
> -		else
> -			result = copy_from_user(ptr, buf, uchunk);
> +		if (uchunk) {
> +			/* For file based kexec, source pages are in kernel memory */
> +			if (image->file_mode)
> +				memcpy(ptr, kbuf, uchunk);
> +			else
> +				result = copy_from_user(ptr, buf, uchunk);
> +			ubytes -= uchunk;
> +			if (image->file_mode)
> +				kbuf += uchunk;
> +			else
> +				buf += uchunk;
> +		}
>  		kunmap_local(ptr);
>  		if (result) {
>  			result = -EFAULT;
>  			goto out;
>  		}
> -		ubytes -= uchunk;
>  		maddr  += mchunk;
> -		if (image->file_mode)
> -			kbuf += mchunk;
> -		else
> -			buf += mchunk;
>  		mbytes -= mchunk;
>  
>  		cond_resched();
> @@ -866,11 +868,18 @@ static int kimage_load_crash_segment(struct kimage *image,
>  			memset(ptr + uchunk, 0, mchunk - uchunk);
>  		}
>  
> -		/* For file based kexec, source pages are in kernel memory */
> -		if (image->file_mode)
> -			memcpy(ptr, kbuf, uchunk);
> -		else
> -			result = copy_from_user(ptr, buf, uchunk);
> +		if (uchunk) {
> +			/* For file based kexec, source pages are in kernel memory */
> +			if (image->file_mode)
> +				memcpy(ptr, kbuf, uchunk);
> +			else
> +				result = copy_from_user(ptr, buf, uchunk);
> +			ubytes -= uchunk;
> +			if (image->file_mode)
> +				kbuf += uchunk;
> +			else
> +				buf += uchunk;
> +		}
>  		kexec_flush_icache_page(page);
>  		kunmap_local(ptr);
>  		arch_kexec_pre_free_pages(page_address(page), 1);
> @@ -878,12 +887,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>  			result = -EFAULT;
>  			goto out;
>  		}
> -		ubytes -= uchunk;
>  		maddr  += mchunk;
> -		if (image->file_mode)
> -			kbuf += mchunk;
> -		else
> -			buf += mchunk;
>  		mbytes -= mchunk;
>  
>  		cond_resched();
> -- 
> 2.25.1
>