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
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
