On 02/05/24 at 06:27am, Eric W. Biederman 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.
ACK.
This makes the code logic much clearer, thanks.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
