* Re: [PATCH] kexec: should use uchunk for user buffer increasing
@ 2024-02-05 12:27 ` Eric W. Biederman
0 siblings, 0 replies; 14+ messages in thread
From: Eric W. Biederman @ 2024-02-05 12:27 UTC (permalink / raw)
To: Baoquan He; +Cc: yang.zhang, kexec, linux-kernel, yang.zhang
Baoquan He <bhe@redhat.com> writes:
> On 01/30/24 at 06:18pm, yang.zhang wrote:
>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] kexec: should use uchunk for user buffer increasing
2024-02-05 12:27 ` Eric W. Biederman
@ 2024-02-05 12:59 ` Baoquan He
-1 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2024-02-05 12:59 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: yang.zhang, kexec, linux-kernel, yang.zhang
On 02/05/24 at 06:27am, Eric W. Biederman wrote:
> Baoquan He <bhe@redhat.com> writes:
>
> > On 01/30/24 at 06:18pm, yang.zhang wrote:
> >> From: "yang.zhang" <yang.zhang@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.
ACK.
This makes the code logic much clearer, thanks.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] kexec: should use uchunk for user buffer increasing
@ 2024-02-05 12:59 ` Baoquan He
0 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2024-02-05 12:59 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: yang.zhang, kexec, linux-kernel, yang.zhang
On 02/05/24 at 06:27am, Eric W. Biederman wrote:
> Baoquan He <bhe@redhat.com> writes:
>
> > On 01/30/24 at 06:18pm, yang.zhang wrote:
> >> From: "yang.zhang" <yang.zhang@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.
ACK.
This makes the code logic much clearer, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re:Re: [PATCH] kexec: should use uchunk for user buffer increasing
2024-02-05 12:27 ` Eric W. Biederman
@ 2024-02-19 2:00 ` yang.zhang
-1 siblings, 0 replies; 14+ messages in thread
From: yang.zhang @ 2024-02-19 2:00 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Baoquan He, kexec, linux-kernel, 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@xmission.com> wrote:
>Baoquan He <bhe@redhat.com> writes:
>
>> On 01/30/24 at 06:18pm, yang.zhang wrote:
>>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re:Re: [PATCH] kexec: should use uchunk for user buffer increasing
@ 2024-02-19 2:00 ` yang.zhang
0 siblings, 0 replies; 14+ messages in thread
From: yang.zhang @ 2024-02-19 2:00 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Baoquan He, kexec, linux-kernel, 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@xmission.com> wrote:
>Baoquan He <bhe@redhat.com> writes:
>
>> On 01/30/24 at 06:18pm, yang.zhang wrote:
>>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] kexec: should use uchunk for user buffer increasing
2024-02-19 2:00 ` yang.zhang
@ 2024-02-19 2:38 ` Baoquan He
-1 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2024-02-19 2:38 UTC (permalink / raw)
To: yang.zhang; +Cc: Eric W. Biederman, kexec, linux-kernel, yang.zhang, akpm
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" <ebiederm@xmission.com> wrote:
> >Baoquan He <bhe@redhat.com> writes:
> >
> >> On 01/30/24 at 06:18pm, yang.zhang wrote:
> >>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] kexec: should use uchunk for user buffer increasing
@ 2024-02-19 2:38 ` Baoquan He
0 siblings, 0 replies; 14+ messages in thread
From: Baoquan He @ 2024-02-19 2:38 UTC (permalink / raw)
To: yang.zhang; +Cc: Eric W. Biederman, kexec, linux-kernel, yang.zhang, akpm
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" <ebiederm@xmission.com> wrote:
> >Baoquan He <bhe@redhat.com> writes:
> >
> >> On 01/30/24 at 06:18pm, yang.zhang wrote:
> >>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re:Re: [PATCH] kexec: should use uchunk for user buffer increasing
2024-02-19 2:38 ` Baoquan He
@ 2024-02-19 9:26 ` yang.zhang
-1 siblings, 0 replies; 14+ messages in thread
From: yang.zhang @ 2024-02-19 9:26 UTC (permalink / raw)
To: Baoquan He; +Cc: Eric W. Biederman, kexec, linux-kernel, yang.zhang, akpm
Thanks, i would post v2 patch.
Could you please provide the email address for andrew.
At 2024-02-19 10:38:22, "Baoquan He" <bhe@redhat.com> wrote:
>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" <ebiederm@xmission.com> wrote:
>> >Baoquan He <bhe@redhat.com> writes:
>> >
>> >> On 01/30/24 at 06:18pm, yang.zhang wrote:
>> >>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re:Re: [PATCH] kexec: should use uchunk for user buffer increasing
@ 2024-02-19 9:26 ` yang.zhang
0 siblings, 0 replies; 14+ messages in thread
From: yang.zhang @ 2024-02-19 9:26 UTC (permalink / raw)
To: Baoquan He; +Cc: Eric W. Biederman, kexec, linux-kernel, yang.zhang, akpm
Thanks, i would post v2 patch.
Could you please provide the email address for andrew.
At 2024-02-19 10:38:22, "Baoquan He" <bhe@redhat.com> wrote:
>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" <ebiederm@xmission.com> wrote:
>> >Baoquan He <bhe@redhat.com> writes:
>> >
>> >> On 01/30/24 at 06:18pm, yang.zhang wrote:
>> >>> From: "yang.zhang" <yang.zhang@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
^ permalink raw reply [flat|nested] 14+ messages in thread