Re:Re: [PATCH] kexec: should use uchunk for user buffer increasing

[yang.zhang]

Thanks for your replies.
Do you have plans to merge the improving code for clarity, or just keep them unchanged.












At 2024-02-05 20:27:33, "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> wrote:
>Baoquan He <bhe@xxxxxxxxxx> writes:
>
>> On 01/30/24 at 06:18pm, yang.zhang wrote:
>>> From: "yang.zhang" <yang.zhang@xxxxxxxxxxxx>
>>> 
>>> Because of alignment requirement in kexec-tools, there is
>>> no problem for user buffer increasing when loading segments.
>>> But when coping, the step is uchunk, so we should use uchunk
>>> not mchunk.
>>
>> In theory, ubytes is <= mbytes. So uchunk is always <= mchunk. If ubytes
>> is exhausted, while there's still remaining mbytes, then uchunk is 0,
>> there's still mchunk stepping forward. If I understand it correctly,
>> this is a good catch. Not sure if Eric has comment on this to confirm.
>
>As far as I can read the code the proposed change is a noop.
>
>I agree it is more correct to not advance the pointers we read from,
>but since we never read from them after that point it does not
>matter.
>
>>
>> static int kimage_load_normal_segment(struct kimage *image,
>>                                          struct kexec_segment *segment)
>> {
>> ......
>>
>>                 ptr += maddr & ~PAGE_MASK;
>>                 mchunk = min_t(size_t, mbytes,
>>                                 PAGE_SIZE - (maddr & ~PAGE_MASK));
>>                 uchunk = min(ubytes, mchunk);
>> ......}
>
>If we are going to improve the code for clarity.  We probably
>want to do something like:
>
>diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>index d08fc7b5db97..1a8b8ce6bf15 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();
>
>And make it exceedingly clear that all of the copying and the rest
>only happens before uchunk goes to zero.  Otherwise we are relying
>on a lot of operations becoming noops when uchunk goes to zero.
>
>Eric