On 02/19/24 at 10:00am, yang.zhang wrote:
>
>
>
> Thanks for your replies.
> Do you have plans to merge the improving code for clarity, or just keep them
> unchanged.
You need post v2 to change those two places as Eric has demonstrated.
Please CC Andrew when you post.
>
> At 2024-02-05 20:27:33, "Eric W. Biederman" <ebied...@xmission.com> wrote:
> >Baoquan He <b...@redhat.com> writes:
> >
> >> On 01/30/24 at 06:18pm, yang.zhang wrote:
> >>> From: "yang.zhang" <yang.zh...@hexintek.com>
> >>>
> >>> 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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
