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

[Baoquan He]

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.