Re:Re: [PATCH v2] kexec: copy only happens before uchunk goes to zero

[yang.zhang]

At 2024-02-22 15:45:38, "Baoquan He" <bhe@xxxxxxxxxx> wrote:
>On 02/20/24 at 10:11am, yang.zhang wrote:
>> From: "yang.zhang" <yang.zhang@xxxxxxxxxxxx>
>> 
>> When load segments, all of the copying and the rest
>> only happens before uchunk goes to zero.
>
>The code change looks good, while the log can be improved with more
>details so that people can more easily get what's wrong and fixed.
>
>===
>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.
>===
>
>Please take above words as reference to udpate patch log and post v3,
>and you can add:
>

>Acked-by: Baoquan He <bhe@xxxxxxxxxx>
Thanks, i update the commits and post v3.>
>> 
>> Signed-off-by: yang.zhang <yang.zhang@xxxxxxxxxxxx>
>> 
>> ---
>> v1 -> v2:
>> - Only copy before uchunk goes to zero
>> 
>> V1: https://lore.kernel.org/lkml/20240130101802.23850-1-gaoshanliukou@xxxxxxx/
>> ---
>>  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
>>