When the split happened, judge whether mem->nr_ranges is equal to
mem->max_nr_ranges. If it is true, return -ENOMEM.
The advantage of doing this is that it can avoid array bounds caused by
some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
extra range for crashkres_low."), reserve both high and low memories for
the crashkernel may cause out of bounds.
On the other hand, move this code before the split to ensure that the
array will not be changed when return error.
Signed-off-by: fuqiang wang <[email protected]>
---
kernel/crash_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index efe87d501c8c..ffdc246cf425 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -611,6 +611,9 @@ int crash_exclude_mem_range(struct crash_mem *mem,
}
if (p_start > start && p_end < end) {
+ /* Split happened */
+ if (mem->nr_ranges == mem->max_nr_ranges)
+ return -ENOMEM;
/* Split original range */
mem->ranges[i].end = p_start - 1;
temp_range.start = p_end + 1;
@@ -626,9 +629,6 @@ int crash_exclude_mem_range(struct crash_mem *mem,
if (!temp_range.end)
return 0;
- /* Split happened */
- if (i == mem->max_nr_ranges - 1)
- return -ENOMEM;
/* Location where new range should go */
j = i + 1;
--
2.42.0
On 11/27/23 at 10:56am, fuqiang wang wrote:
> When the split happened, judge whether mem->nr_ranges is equal to
> mem->max_nr_ranges. If it is true, return -ENOMEM.
>
> The advantage of doing this is that it can avoid array bounds caused by
> some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
> extra range for crashkres_low."), reserve both high and low memories for
> the crashkernel may cause out of bounds.
>
> On the other hand, move this code before the split to ensure that the
> array will not be changed when return error.
If out of array boundary is caused, means the laoding failed, whether
the out of boundary happened or not. I don't see how this code change
makes sense. Do I miss anything?
Thanks
Baoquan
>
> Signed-off-by: fuqiang wang <[email protected]>
> ---
> kernel/crash_core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index efe87d501c8c..ffdc246cf425 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -611,6 +611,9 @@ int crash_exclude_mem_range(struct crash_mem *mem,
> }
>
> if (p_start > start && p_end < end) {
> + /* Split happened */
> + if (mem->nr_ranges == mem->max_nr_ranges)
> + return -ENOMEM;
> /* Split original range */
> mem->ranges[i].end = p_start - 1;
> temp_range.start = p_end + 1;
> @@ -626,9 +629,6 @@ int crash_exclude_mem_range(struct crash_mem *mem,
> if (!temp_range.end)
> return 0;
>
> - /* Split happened */
> - if (i == mem->max_nr_ranges - 1)
> - return -ENOMEM;
>
> /* Location where new range should go */
> j = i + 1;
> --
> 2.42.0
>
>
> _______________________________________________
> kexec mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kexec
>
On 2023/11/30 15:44, Baoquan He wrote:
> On 11/27/23 at 10:56am, fuqiang wang wrote:
>> When the split happened, judge whether mem->nr_ranges is equal to
>> mem->max_nr_ranges. If it is true, return -ENOMEM.
>>
>> The advantage of doing this is that it can avoid array bounds caused by
>> some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
>> extra range for crashkres_low."), reserve both high and low memories for
>> the crashkernel may cause out of bounds.
>>
>> On the other hand, move this code before the split to ensure that the
>> array will not be changed when return error.
> If out of array boundary is caused, means the laoding failed, whether
> the out of boundary happened or not. I don't see how this code change
> makes sense. Do I miss anything?
>
> Thanks
> Baoquan
>
Hi baoquan,
In some configurations, out of bounds may not cause crash_exclude_mem_range()
returns error, then the load will succeed.
E.g.
There is a cmem before execute crash_exclude_mem_range():
cmem = {
max_nr_ranges = 3
nr_ranges = 2
ranges = {
{start = 1, end = 1000}
{start = 1001, end = 2000}
}
}
After executing twice crash_exclude_mem_range() with the start/end params
100/200, 300/400 respectively, the cmem will be:
cmem = {
max_nr_ranges = 3
nr_ranges = 4 <== nr_ranges > max_nr_ranges
ranges = {
{start = 1, end = 99 }
{start = 201, end = 299 }
{start = 401, end = 1000}
{start = 1001, end = 2000} <== OUT OF BOUNDS
}
}
When an out of bounds occurs during the second execution, the function will not
return error.
Additionally, when the function returns error, means the load failed. It seems
meaningless to keep the original data unchanged. But in my opinion, this will
make this function more rigorous and more versatile. (However, I am not sure if
it is self-defeating and I hope to receive more suggestions).
Thanks
fuqiang
>> Signed-off-by: fuqiang wang <[email protected]>
>> ---
>> kernel/crash_core.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index efe87d501c8c..ffdc246cf425 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -611,6 +611,9 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>> }
>>
>> if (p_start > start && p_end < end) {
>> + /* Split happened */
>> + if (mem->nr_ranges == mem->max_nr_ranges)
>> + return -ENOMEM;
>> /* Split original range */
>> mem->ranges[i].end = p_start - 1;
>> temp_range.start = p_end + 1;
>> @@ -626,9 +629,6 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>> if (!temp_range.end)
>> return 0;
>>
>> - /* Split happened */
>> - if (i == mem->max_nr_ranges - 1)
>> - return -ENOMEM;
>>
>> /* Location where new range should go */
>> j = i + 1;
>> --
>> 2.42.0
>>
>>
>> _______________________________________________
>> kexec mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/kexec
>>
On 11/30/23 at 09:20pm, fuqiang wang wrote:
>
> On 2023/11/30 15:44, Baoquan He wrote:
> > On 11/27/23 at 10:56am, fuqiang wang wrote:
> > > When the split happened, judge whether mem->nr_ranges is equal to
> > > mem->max_nr_ranges. If it is true, return -ENOMEM.
> > >
> > > The advantage of doing this is that it can avoid array bounds caused by
> > > some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
> > > extra range for crashkres_low."), reserve both high and low memories for
> > > the crashkernel may cause out of bounds.
> > >
> > > On the other hand, move this code before the split to ensure that the
> > > array will not be changed when return error.
> > If out of array boundary is caused, means the laoding failed, whether
> > the out of boundary happened or not. I don't see how this code change
> > makes sense. Do I miss anything?
> >
> > Thanks
> > Baoquan
> >
> Hi baoquan,
>
> In some configurations, out of bounds may not cause crash_exclude_mem_range()
> returns error, then the load will succeed.
>
> E.g.
> There is a cmem before execute crash_exclude_mem_range():
>
> ? cmem = {
> ??? max_nr_ranges = 3
> ??? nr_ranges = 2
> ??? ranges = {
> ?????? {start = 1,????? end = 1000}
> ?????? {start = 1001,??? end = 2000}
> ??? }
> ? }
>
> After executing twice crash_exclude_mem_range() with the start/end params
> 100/200, 300/400 respectively, the cmem will be:
>
> ? cmem = {
> ??? max_nr_ranges = 3
> ??? nr_ranges = 4??????????????????? <== nr_ranges > max_nr_ranges
> ??? ranges = {
> ????? {start = 1,?????? end = 99? }
> ????? {start = 201,???? end = 299 }
> ????? {start = 401,???? end = 1000}
> ????? {start = 1001,??? end = 2000}? <== OUT OF BOUNDS
> ??? }
> ? }
>
> When an out of bounds occurs during the second execution, the function will not
> return error.
>
> Additionally, when the function returns error, means the load failed. It seems
> meaningless to keep the original data unchanged. But in my opinion, this will
> make this function more rigorous and more versatile. (However, I am not sure if
> it is self-defeating and I hope to receive more suggestions).
Sorry for late reply.
I checked the code again, there seems to be cases out of bounds occur
very possiblly. We may need to enlarge the cmem array to avoid the risk.
In below draft code, we need add another slot to exclude the low 1M area
when preparing elfcorehdr. And to exclude the elf header region from
crash kernel region, we need create the cmem with 2 slots.
With these change, we can absolutely avoid out of bounds occurence.
What do you think?
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 1715e5f06a59..21facabcf699 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -147,10 +147,10 @@ static struct crash_mem *fill_up_crash_elf_data(void)
return NULL;
/*
- * Exclusion of crash region and/or crashk_low_res may cause
- * another range split. So add extra two slots here.
+ * Exclusion of low 1M, crash region and/or crashk_low_res may
+ * cause another range split. So add extra two slots here.
*/
- nr_ranges += 2;
+ nr_ranges += 3;
cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
if (!cmem)
return NULL;
@@ -282,7 +282,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
struct crash_memmap_data cmd;
struct crash_mem *cmem;
- cmem = vzalloc(struct_size(cmem, ranges, 1));
+ cmem = vzalloc(struct_size(cmem, ranges, 2));
if (!cmem)
return -ENOMEM;
在 2023/12/13 12:44, Baoquan He 写道:
> On 11/30/23 at 09:20pm, fuqiang wang wrote:
>> On 2023/11/30 15:44, Baoquan He wrote:
>>> On 11/27/23 at 10:56am, fuqiang wang wrote:
>>>> When the split happened, judge whether mem->nr_ranges is equal to
>>>> mem->max_nr_ranges. If it is true, return -ENOMEM.
>>>>
>>>> The advantage of doing this is that it can avoid array bounds caused by
>>>> some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
>>>> extra range for crashkres_low."), reserve both high and low memories for
>>>> the crashkernel may cause out of bounds.
>>>>
>>>> On the other hand, move this code before the split to ensure that the
>>>> array will not be changed when return error.
>>> If out of array boundary is caused, means the laoding failed, whether
>>> the out of boundary happened or not. I don't see how this code change
>>> makes sense. Do I miss anything?
>>>
>>> Thanks
>>> Baoquan
>>>
>> Hi baoquan,
>>
>> In some configurations, out of bounds may not cause crash_exclude_mem_range()
>> returns error, then the load will succeed.
>>
>> E.g.
>> There is a cmem before execute crash_exclude_mem_range():
>>
>> cmem = {
>> max_nr_ranges = 3
>> nr_ranges = 2
>> ranges = {
>> {start = 1, end = 1000}
>> {start = 1001, end = 2000}
>> }
>> }
>>
>> After executing twice crash_exclude_mem_range() with the start/end params
>> 100/200, 300/400 respectively, the cmem will be:
>>
>> cmem = {
>> max_nr_ranges = 3
>> nr_ranges = 4 <== nr_ranges > max_nr_ranges
>> ranges = {
>> {start = 1, end = 99 }
>> {start = 201, end = 299 }
>> {start = 401, end = 1000}
>> {start = 1001, end = 2000} <== OUT OF BOUNDS
>> }
>> }
>>
>> When an out of bounds occurs during the second execution, the function will not
>> return error.
>>
>> Additionally, when the function returns error, means the load failed. It seems
>> meaningless to keep the original data unchanged. But in my opinion, this will
>> make this function more rigorous and more versatile. (However, I am not sure if
>> it is self-defeating and I hope to receive more suggestions).
> Sorry for late reply.
>
> I checked the code again, there seems to be cases out of bounds occur
> very possiblly. We may need to enlarge the cmem array to avoid the risk.
>
> In below draft code, we need add another slot to exclude the low 1M area
> when preparing elfcorehdr. And to exclude the elf header region from
> crash kernel region, we need create the cmem with 2 slots.
>
> With these change, we can absolutely avoid out of bounds occurence.
> What do you think?
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 1715e5f06a59..21facabcf699 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -147,10 +147,10 @@ static struct crash_mem *fill_up_crash_elf_data(void)
> return NULL;
>
> /*
> - * Exclusion of crash region and/or crashk_low_res may cause
> - * another range split. So add extra two slots here.
> + * Exclusion of low 1M, crash region and/or crashk_low_res may
> + * cause another range split. So add extra two slots here.
> */
> - nr_ranges += 2;
> + nr_ranges += 3;
> cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> if (!cmem)
> return NULL;
Hi baoquan,
Exclusion of low 1M may not cause new region. Because when calling
crash_exclude_mem_range(), the start parameter is 0 and the condition for
splitting a new region is that the start, end parameters are both in a certain
existing region in cmem and cannot be equal to existing region's start or end.
Obviously, start (0) cannot meet this condition.
> @@ -282,7 +282,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> struct crash_memmap_data cmd;
> struct crash_mem *cmem;
>
> - cmem = vzalloc(struct_size(cmem, ranges, 1));
> + cmem = vzalloc(struct_size(cmem, ranges, 2));
> if (!cmem)
> return -ENOMEM;
>
>
Yes, you are right. Exclude the elf header region from crash kernel region may
cause split a new region. And there seems to be another issue with this code
path: Before calling crash_exclude_mem_range(), cmem->max_nr_ranges was not
initialized.
In my opinion, these change can absolutely avoid out of bounds occurence. But
when we forget to modify max_nr_ranges due to a mistakes in the future, is it
better to report it by returning an error through crash_exclude_mem_range().
What do you think about it?
Thanks
fuqiang
On 12/13/23 at 09:10pm, fuqiang wang wrote:
> 在 2023/12/13 12:44, Baoquan He 写道:
>
> > On 11/30/23 at 09:20pm, fuqiang wang wrote:
> > > On 2023/11/30 15:44, Baoquan He wrote:
> > > > On 11/27/23 at 10:56am, fuqiang wang wrote:
> > > > > When the split happened, judge whether mem->nr_ranges is equal to
> > > > > mem->max_nr_ranges. If it is true, return -ENOMEM.
> > > > >
> > > > > The advantage of doing this is that it can avoid array bounds caused by
> > > > > some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
> > > > > extra range for crashkres_low."), reserve both high and low memories for
> > > > > the crashkernel may cause out of bounds.
> > > > >
> > > > > On the other hand, move this code before the split to ensure that the
> > > > > array will not be changed when return error.
> > > > If out of array boundary is caused, means the laoding failed, whether
> > > > the out of boundary happened or not. I don't see how this code change
> > > > makes sense. Do I miss anything?
> > > >
> > > > Thanks
> > > > Baoquan
> > > >
> > > Hi baoquan,
> > >
> > > In some configurations, out of bounds may not cause crash_exclude_mem_range()
> > > returns error, then the load will succeed.
> > >
> > > E.g.
> > > There is a cmem before execute crash_exclude_mem_range():
> > >
> > > cmem = {
> > > max_nr_ranges = 3
> > > nr_ranges = 2
> > > ranges = {
> > > {start = 1, end = 1000}
> > > {start = 1001, end = 2000}
> > > }
> > > }
> > >
> > > After executing twice crash_exclude_mem_range() with the start/end params
> > > 100/200, 300/400 respectively, the cmem will be:
> > >
> > > cmem = {
> > > max_nr_ranges = 3
> > > nr_ranges = 4 <== nr_ranges > max_nr_ranges
> > > ranges = {
> > > {start = 1, end = 99 }
> > > {start = 201, end = 299 }
> > > {start = 401, end = 1000}
> > > {start = 1001, end = 2000} <== OUT OF BOUNDS
> > > }
> > > }
> > >
> > > When an out of bounds occurs during the second execution, the function will not
> > > return error.
> > >
> > > Additionally, when the function returns error, means the load failed. It seems
> > > meaningless to keep the original data unchanged. But in my opinion, this will
> > > make this function more rigorous and more versatile. (However, I am not sure if
> > > it is self-defeating and I hope to receive more suggestions).
> > Sorry for late reply.
> >
> > I checked the code again, there seems to be cases out of bounds occur
> > very possiblly. We may need to enlarge the cmem array to avoid the risk.
> >
> > In below draft code, we need add another slot to exclude the low 1M area
> > when preparing elfcorehdr. And to exclude the elf header region from
> > crash kernel region, we need create the cmem with 2 slots.
> >
> > With these change, we can absolutely avoid out of bounds occurence.
> > What do you think?
> >
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 1715e5f06a59..21facabcf699 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -147,10 +147,10 @@ static struct crash_mem *fill_up_crash_elf_data(void)
> > return NULL;
> > /*
> > - * Exclusion of crash region and/or crashk_low_res may cause
> > - * another range split. So add extra two slots here.
> > + * Exclusion of low 1M, crash region and/or crashk_low_res may
> > + * cause another range split. So add extra two slots here.
> > */
> > - nr_ranges += 2;
> > + nr_ranges += 3;
> > cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> > if (!cmem)
> > return NULL;
> Hi baoquan,
>
> Exclusion of low 1M may not cause new region. Because when calling
> crash_exclude_mem_range(), the start parameter is 0 and the condition for
> splitting a new region is that the start, end parameters are both in a certain
> existing region in cmem and cannot be equal to existing region's start or end.
> Obviously, start (0) cannot meet this condition.
OK, this is an special case.
> > @@ -282,7 +282,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> > struct crash_memmap_data cmd;
> > struct crash_mem *cmem;
> > - cmem = vzalloc(struct_size(cmem, ranges, 1));
> > + cmem = vzalloc(struct_size(cmem, ranges, 2));
> > if (!cmem)
> > return -ENOMEM;
> >
> Yes, you are right. Exclude the elf header region from crash kernel region may
> cause split a new region. And there seems to be another issue with this code
> path: Before calling crash_exclude_mem_range(), cmem->max_nr_ranges was not
> initialized.
Yeah, the init need be added.
>
> In my opinion, these change can absolutely avoid out of bounds occurence. But
> when we forget to modify max_nr_ranges due to a mistakes in the future, is it
> better to report it by returning an error through crash_exclude_mem_range().
> What do you think about it?
I don't see the difference between your patch and the current code.
Please see my comment in your earlier example.
On 11/30/23 at 09:20pm, fuqiang wang wrote:
>
> On 2023/11/30 15:44, Baoquan He wrote:
> > On 11/27/23 at 10:56am, fuqiang wang wrote:
> > > When the split happened, judge whether mem->nr_ranges is equal to
> > > mem->max_nr_ranges. If it is true, return -ENOMEM.
> > >
> > > The advantage of doing this is that it can avoid array bounds caused by
> > > some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
> > > extra range for crashkres_low."), reserve both high and low memories for
> > > the crashkernel may cause out of bounds.
> > >
> > > On the other hand, move this code before the split to ensure that the
> > > array will not be changed when return error.
> > If out of array boundary is caused, means the laoding failed, whether
> > the out of boundary happened or not. I don't see how this code change
> > makes sense. Do I miss anything?
> >
> > Thanks
> > Baoquan
> >
> Hi baoquan,
>
> In some configurations, out of bounds may not cause crash_exclude_mem_range()
> returns error, then the load will succeed.
>
> E.g.
> There is a cmem before execute crash_exclude_mem_range():
>
> ? cmem = {
> ??? max_nr_ranges = 3
> ??? nr_ranges = 2
> ??? ranges = {
> ?????? {start = 1,????? end = 1000}
> ?????? {start = 1001,??? end = 2000}
> ??? }
> ? }
>
> After executing twice crash_exclude_mem_range() with the start/end params
> 100/200, 300/400 respectively, the cmem will be:
>
> ? cmem = {
> ??? max_nr_ranges = 3
> ??? nr_ranges = 4??????????????????? <== nr_ranges > max_nr_ranges
> ??? ranges = {
> ????? {start = 1,?????? end = 99? }
> ????? {start = 201,???? end = 299 }
> ????? {start = 401,???? end = 1000}
> ????? {start = 1001,??? end = 2000}? <== OUT OF BOUNDS
> ??? }
> ? }
Let me borrow your example and copy them here, but I will switch the
order of start/end params 100/200, 300/400 executing at below:
There is a cmem before execute crash_exclude_mem_range():
? cmem = {
??? max_nr_ranges = 3
??? nr_ranges = 2
??? ranges = {
?????? {start = 1,????? end = 1000}
?????? {start = 1001,??? end = 2000}
??? }
? }
After executing twice crash_exclude_mem_range() with the start/end params
300/400, the cmem will be:
? cmem = {
??? max_nr_ranges = 3
??? nr_ranges = 3??????????????????? <== nr_ranges == max_nr_ranges
??? ranges = {
????? {start = 1,?????? end = 299? } i=0
????? {start = 401,???? end = 1000} i=1
????? {start = 1001,??? end = 2000}? i=2
??? }
? }
When it's executing the 100/200 excluding, we have:
? cmem = {
??? max_nr_ranges = 3
??? nr_ranges = 4??????????????????? <== nr_ranges > max_nr_ranges
??? ranges = {
????? {start = 1,?????? end = 99? } i=0
????? {start = 401,???? end = 1000}
????? {start = 1001,??? end = 2000}?
??? }
? }
Then splitting happened, i == 0, then for loop is broken and jump out.
Then we have the condition checking here:
/* Split happened */
if (i == mem->max_nr_ranges - 1)
return -ENOMEM;
Obviously the conditonal checking is incorrect (given the i == 0 in
above case), it should be
/* Split happened */
if (mem->nr_ranges == mem->max_nr_ranges)
return -ENOMEM;
So, now there are two things which need be combed up in
crash_exclude_mem_range():
1) the above conditional check is incorrect, need be fixed;
2) whether we need have the cmem->ranges[] partly changed, or keep it
unchanged when OOB happened;
And also the incorrect handling in crash_setup_memmap_entries():
1) the insufficient array slot in crash_setup_memmap_entries();
2) the uninitialized cmem->max_nr_ranges;
>
> When an out of bounds occurs during the second execution, the function will not
> return error.
>
> Additionally, when the function returns error, means the load failed. It seems
> meaningless to keep the original data unchanged. But in my opinion, this will
> make this function more rigorous and more versatile. (However, I am not sure if
> it is self-defeating and I hope to receive more suggestions).
>
> Thanks
> fuqiang
>
>
> > > Signed-off-by: fuqiang wang <[email protected]>
> > > ---
> > > kernel/crash_core.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > > index efe87d501c8c..ffdc246cf425 100644
> > > --- a/kernel/crash_core.c
> > > +++ b/kernel/crash_core.c
> > > @@ -611,6 +611,9 @@ int crash_exclude_mem_range(struct crash_mem *mem,
> > > }
> > > if (p_start > start && p_end < end) {
> > > + /* Split happened */
> > > + if (mem->nr_ranges == mem->max_nr_ranges)
> > > + return -ENOMEM;
> > > /* Split original range */
> > > mem->ranges[i].end = p_start - 1;
> > > temp_range.start = p_end + 1;
> > > @@ -626,9 +629,6 @@ int crash_exclude_mem_range(struct crash_mem *mem,
> > > if (!temp_range.end)
> > > return 0;
> > > - /* Split happened */
> > > - if (i == mem->max_nr_ranges - 1)
> > > - return -ENOMEM;
> > > /* Location where new range should go */
> > > j = i + 1;
> > > --
> > > 2.42.0
> > >
> > >
> > > _______________________________________________
> > > kexec mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/kexec
> > >
>
在 2023/12/14 18:29, Baoquan He 写道:
> On 11/30/23 at 09:20pm, fuqiang wang wrote:
>> On 2023/11/30 15:44, Baoquan He wrote:
>>> On 11/27/23 at 10:56am, fuqiang wang wrote:
>>>> When the split happened, judge whether mem->nr_ranges is equal to
>>>> mem->max_nr_ranges. If it is true, return -ENOMEM.
>>>>
>>>> The advantage of doing this is that it can avoid array bounds caused by
>>>> some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
>>>> extra range for crashkres_low."), reserve both high and low memories for
>>>> the crashkernel may cause out of bounds.
>>>>
>>>> On the other hand, move this code before the split to ensure that the
>>>> array will not be changed when return error.
>>> If out of array boundary is caused, means the laoding failed, whether
>>> the out of boundary happened or not. I don't see how this code change
>>> makes sense. Do I miss anything?
>>>
>>> Thanks
>>> Baoquan
>>>
>> Hi baoquan,
>>
>> In some configurations, out of bounds may not cause crash_exclude_mem_range()
>> returns error, then the load will succeed.
>>
>> E.g.
>> There is a cmem before execute crash_exclude_mem_range():
>>
>> cmem = {
>> max_nr_ranges = 3
>> nr_ranges = 2
>> ranges = {
>> {start = 1, end = 1000}
>> {start = 1001, end = 2000}
>> }
>> }
>>
>> After executing twice crash_exclude_mem_range() with the start/end params
>> 100/200, 300/400 respectively, the cmem will be:
>>
>> cmem = {
>> max_nr_ranges = 3
>> nr_ranges = 4 <== nr_ranges > max_nr_ranges
>> ranges = {
>> {start = 1, end = 99 }
>> {start = 201, end = 299 }
>> {start = 401, end = 1000}
>> {start = 1001, end = 2000} <== OUT OF BOUNDS
>> }
>> }
> Let me borrow your example and copy them here, but I will switch the
> order of start/end params 100/200, 300/400 executing at below:
>
> There is a cmem before execute crash_exclude_mem_range():
>
> cmem = {
> max_nr_ranges = 3
> nr_ranges = 2
> ranges = {
> {start = 1, end = 1000}
> {start = 1001, end = 2000}
> }
> }
>
> After executing twice crash_exclude_mem_range() with the start/end params
> 300/400, the cmem will be:
>
> cmem = {
> max_nr_ranges = 3
> nr_ranges = 3 <== nr_ranges == max_nr_ranges
> ranges = {
> {start = 1, end = 299 } i=0
> {start = 401, end = 1000} i=1
> {start = 1001, end = 2000} i=2
> }
> }
> When it's executing the 100/200 excluding, we have:
>
> cmem = {
> max_nr_ranges = 3
> nr_ranges = 4 <== nr_ranges > max_nr_ranges
> ranges = {
> {start = 1, end = 99 } i=0
> {start = 401, end = 1000}
> {start = 1001, end = 2000}
> }
> }
>
> Then splitting happened, i == 0, then for loop is broken and jump out.
> Then we have the condition checking here:
>
> /* Split happened */
> if (i == mem->max_nr_ranges - 1)
> return -ENOMEM;
>
> Obviously the conditonal checking is incorrect (given the i == 0 in
> above case), it should be
>
> /* Split happened */
> if (mem->nr_ranges == mem->max_nr_ranges)
> return -ENOMEM;
>
> So, now there are two things which need be combed up in
> crash_exclude_mem_range():
>
> 1) the above conditional check is incorrect, need be fixed;
> 2) whether we need have the cmem->ranges[] partly changed, or keep it
> unchanged when OOB happened;
Hi baoquan,
I'm sorry, I would like to confirm if I misunderstood the meaning of your
comment or not. What you mean is that you have agreed to merge the patch, but
before that, it needs to be explained in detail in the commit message. Is this
understanding correct?
> And also the incorrect handling in crash_setup_memmap_entries():
> 1) the insufficient array slot in crash_setup_memmap_entries();
> 2) the uninitialized cmem->max_nr_ranges;
>
If this patch can be merged, the issue of the uninitialized cmem->max_nr_ranges
must be resolved before the patch is merged because this patch requires a
initialized max_nr_ranges value. I am willing to take on the task of addressing
those issues.
Thanks
fuqiang
>> When an out of bounds occurs during the second execution, the function will not
>> return error.
>>
>> Additionally, when the function returns error, means the load failed. It seems
>> meaningless to keep the original data unchanged. But in my opinion, this will
>> make this function more rigorous and more versatile. (However, I am not sure if
>> it is self-defeating and I hope to receive more suggestions).
>>
>> Thanks
>> fuqiang
>>
>>
>>>> Signed-off-by: fuqiang wang <[email protected]>
>>>> ---
>>>> kernel/crash_core.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>>>> index efe87d501c8c..ffdc246cf425 100644
>>>> --- a/kernel/crash_core.c
>>>> +++ b/kernel/crash_core.c
>>>> @@ -611,6 +611,9 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>>>> }
>>>> if (p_start > start && p_end < end) {
>>>> + /* Split happened */
>>>> + if (mem->nr_ranges == mem->max_nr_ranges)
>>>> + return -ENOMEM;
>>>> /* Split original range */
>>>> mem->ranges[i].end = p_start - 1;
>>>> temp_range.start = p_end + 1;
>>>> @@ -626,9 +629,6 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>>>> if (!temp_range.end)
>>>> return 0;
>>>> - /* Split happened */
>>>> - if (i == mem->max_nr_ranges - 1)
>>>> - return -ENOMEM;
>>>> /* Location where new range should go */
>>>> j = i + 1;
>>>> --
>>>> 2.42.0
>>>>
>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> [email protected]
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>>
Hi fuqiang,
Yesterday, I posted two patches that happen to address the bugs you an Baoquan
are currently discussing here, I wasn't aware that you both were also working
on fixing these issues.
Baoquan suggested I talk to you about it.
If you're interested, you can take a look at my patches and review them to see
if there are any issues. If everything is fine, and if you're willing, you can
also add a 'Reviewed-by' tag there.
Sincerely,
Yuntao
Hi fuqiang,
Yesterday, I posted two patches that happen to address the bugs you an Baoquan
are currently discussing here, I wasn't aware that you both were also working
on fixing these issues.
Baoquan suggested I talk to you about it.
If you're interested, you can take a look at my patches and review them to see
if there are any issues. If everything is fine, and if you're willing, you can
also add a 'Reviewed-by' tag there.
The following link is for the two patches I posted yesterday:
https://lore.kernel.org/lkml/[email protected]/t/#u
Sincerely,
Yuntao
在 2023/12/19 10:47, Yuntao Wang 写道:
> Hi fuqiang,
>
> Yesterday, I posted two patches that happen to address the bugs you an Baoquan
> are currently discussing here, I wasn't aware that you both were also working
> on fixing these issues.
>
> Baoquan suggested I talk to you about it.
>
> If you're interested, you can take a look at my patches and review them to see
> if there are any issues. If everything is fine, and if you're willing, you can
> also add a 'Reviewed-by' tag there.
>
> The following link is for the two patches I posted yesterday:
>
> https://lore.kernel.org/lkml/[email protected]/t/#u
>
> Sincerely,
> Yuntao
Hi Yuntao,
I'm glad you've also noticed this issue. But I'm sorry, I want to solve this
problem myself because this is my first time posting a patch in the community,
and I cherish this opportunity very much.
I have carefully reviewed your patch. There is some changes where my views differ
from yours:
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..3be46f4b441e 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -282,10 +282,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
struct crash_memmap_data cmd;
struct crash_mem *cmem;
- cmem = vzalloc(struct_size(cmem, ranges, 1));
- if (!cmem)
- return -ENOMEM;
-
memset(&cmd, 0, sizeof(struct crash_memmap_data));
cmd.params = params;
@@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
}
/* Exclude some ranges from crashk_res and add rest to memmap */
+ cmem = vzalloc(struct_size(cmem, ranges, 1));
+ if (!cmem)
+ return -ENOMEM;
+ cmem->max_nr_ranges = 1;
+
ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
if (ret)
goto out;
1. I don't feel very good that you have moved vzalloc() to in front of
memmap_exclude_ranges. Because if memory allocation fails, there is no need to
do anything else afterwards.
2. The cmem->max_nr_ranges should be set to 2. Because in
memmap_exclude_ranges, a cmem->ranges[] will be filled in and if a split occurs
later, another one will be added.
Thanks
fuqiang
On Tue, 19 Dec 2023 11:50:32 +0800, fuqiang wang <[email protected]> wrote:
> 在 2023/12/19 10:47, Yuntao Wang 写道:
>
> > Hi fuqiang,
> >
> > Yesterday, I posted two patches that happen to address the bugs you an Baoquan
> > are currently discussing here, I wasn't aware that you both were also working
> > on fixing these issues.
> >
> > Baoquan suggested I talk to you about it.
> >
> > If you're interested, you can take a look at my patches and review them to see
> > if there are any issues. If everything is fine, and if you're willing, you can
> > also add a 'Reviewed-by' tag there.
> >
> > The following link is for the two patches I posted yesterday:
> >
> > https://lore.kernel.org/lkml/[email protected]/t/#u
> >
> > Sincerely,
> > Yuntao
>
> Hi Yuntao,
>
> I'm glad you've also noticed this issue. But I'm sorry, I want to solve this
> problem myself because this is my first time posting a patch in the community,
> and I cherish this opportunity very much.
I can truly understand your feelings because I still remember how thrilled I
was when my first patch got merged. So keep it up!
>
> I have carefully reviewed your patch. There is some changes where my views differ
> from yours:
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index c92d88680dbf..3be46f4b441e 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -282,10 +282,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> struct crash_memmap_data cmd;
> struct crash_mem *cmem;
>
> - cmem = vzalloc(struct_size(cmem, ranges, 1));
> - if (!cmem)
> - return -ENOMEM;
> -
> memset(&cmd, 0, sizeof(struct crash_memmap_data));
> cmd.params = params;
>
> @@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> }
>
> /* Exclude some ranges from crashk_res and add rest to memmap */
> + cmem = vzalloc(struct_size(cmem, ranges, 1));
> + if (!cmem)
> + return -ENOMEM;
> + cmem->max_nr_ranges = 1;
> +
> ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
> if (ret)
> goto out;
>
> 1. I don't feel very good that you have moved vzalloc() to in front of
> memmap_exclude_ranges. Because if memory allocation fails, there is no need to
> do anything else afterwards.
I moved it here because only memmap_exclude_ranges() and the code below it use cmem.
I think it is a good practice to put related code together, which also improves
code readability.
>
> 2. The cmem->max_nr_ranges should be set to 2. Because in
> memmap_exclude_ranges, a cmem->ranges[] will be filled in and if a split occurs
> later, another one will be added.
With the current code, image->elf_load_addr should be equal to crashk_res.start,
so split will not occur in crash_exclude_mem_range(). Therefore, setting
cmem->max_nr_ranges to 1 is safe.
>
> Thanks
> fuqiang
On Tue, 19 Dec 2023 16:55:16 +0800, fuqiang wang <[email protected]> wrote:
> Thank you very much for your patient comment. This change does indeed improve
> readability. But as a combination of these two, how do you feel about moving
> crash_setup_memmap_entries() behind vzalloc().
I don't quite understand what you're trying to express.
> The image->elf_load_addr is determined by arch_kexec_locate_mem_hole(), this
> function can ensure that the value is within the range of [crashk_res.start,
> crashk_res.end), but it seems that it cannot guarantee that its value will
> always be equal to crashk_res.start. Perhaps I have some omissions, please
> point them out.
Because elfcorehdr is the first one and only one that allocates memory from the
starting address of crashk_res, and the starting address of crashk_res meets
the alignment requirement of elfcorehdr.
elfcorehdr requires 4k alignment, and the starting address of crashk_res is
16M aligned.
Therefore, image->elf_load_addr should be equal to crashk_res.start.
在 2023/12/19 13:29, Yuntao Wang 写道:
> On Tue, 19 Dec 2023 11:50:32 +0800, fuqiang wang <[email protected]> wrote:
>> 在 2023/12/19 10:47, Yuntao Wang 写道:
>>
>>> Hi fuqiang,
>>>
>>> Yesterday, I posted two patches that happen to address the bugs you an Baoquan
>>> are currently discussing here, I wasn't aware that you both were also working
>>> on fixing these issues.
>>>
>>> Baoquan suggested I talk to you about it.
>>>
>>> If you're interested, you can take a look at my patches and review them to see
>>> if there are any issues. If everything is fine, and if you're willing, you can
>>> also add a 'Reviewed-by' tag there.
>>>
>>> The following link is for the two patches I posted yesterday:
>>>
>>> https://lore.kernel.org/lkml/[email protected]/t/#u
>>>
>>> Sincerely,
>>> Yuntao
>> Hi Yuntao,
>>
>> I'm glad you've also noticed this issue. But I'm sorry, I want to solve this
>> problem myself because this is my first time posting a patch in the community,
>> and I cherish this opportunity very much.
> I can truly understand your feelings because I still remember how thrilled I
> was when my first patch got merged. So keep it up!
Hi Yuntao,
Thanks for your understanding and encourage. :)
>> I have carefully reviewed your patch. There is some changes where my views differ
>> from yours:
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index c92d88680dbf..3be46f4b441e 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -282,10 +282,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>> struct crash_memmap_data cmd;
>> struct crash_mem *cmem;
>>
>> - cmem = vzalloc(struct_size(cmem, ranges, 1));
>> - if (!cmem)
>> - return -ENOMEM;
>> -
>> memset(&cmd, 0, sizeof(struct crash_memmap_data));
>> cmd.params = params;
>>
>> @@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>> }
>>
>> /* Exclude some ranges from crashk_res and add rest to memmap */
>> + cmem = vzalloc(struct_size(cmem, ranges, 1));
>> + if (!cmem)
>> + return -ENOMEM;
>> + cmem->max_nr_ranges = 1;
>> +
>> ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
>> if (ret)
>> goto out;
>>
>> 1. I don't feel very good that you have moved vzalloc() to in front of
>> memmap_exclude_ranges. Because if memory allocation fails, there is no need to
>> do anything else afterwards.
> I moved it here because only memmap_exclude_ranges() and the code below it use cmem.
>
> I think it is a good practice to put related code together, which also improves
> code readability.
Thank you very much for your patient comment. This change does indeed improve
readability. But as a combination of these two, how do you feel about moving
crash_setup_memmap_entries() behind vzalloc().
>> 2. The cmem->max_nr_ranges should be set to 2. Because in
>> memmap_exclude_ranges, a cmem->ranges[] will be filled in and if a split occurs
>> later, another one will be added.
> With the current code, image->elf_load_addr should be equal to crashk_res.start,
> so split will not occur in crash_exclude_mem_range(). Therefore, setting
> cmem->max_nr_ranges to 1 is safe.
The image->elf_load_addr is determined by arch_kexec_locate_mem_hole(), this
function can ensure that the value is within the range of [crashk_res.start,
crashk_res.end), but it seems that it cannot guarantee that its value will
always be equal to crashk_res.start. Perhaps I have some omissions, please
point them out.
Thanks
fuqiang
>> Thanks
>> fuqiang
在 2023/12/19 18:39, Yuntao Wang 写道:
> On Tue, 19 Dec 2023 16:55:16 +0800, fuqiang wang <[email protected]> wrote:
>
>> Thank you very much for your patient comment. This change does indeed improve
>> readability. But as a combination of these two, how do you feel about moving
>> crash_setup_memmap_entries() behind vzalloc().
> I don't quite understand what you're trying to express.
Hi Yuntao,
I make the following changes based on your patch. This change can increase code
readability on one hand, On the other hand, if these functions return errors,
the rest process of crash_setup_memmap_entries() can be skipped.
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..67a974c041b9 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -285,6 +285,12 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
cmem = vzalloc(struct_size(cmem, ranges, 1));
if (!cmem)
return -ENOMEM;
+ cmem->max_nr_ranges = 1;
+
+ /* Exclude some ranges from crashk_res and add rest to memmap */
+ ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
+ if (ret)
+ goto out;
memset(&cmd, 0, sizeof(struct crash_memmap_data));
cmd.params = params;
@@ -320,11 +326,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
add_e820_entry(params, &ei);
}
- /* Exclude some ranges from crashk_res and add rest to memmap */
- ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
- if (ret)
- goto out;
-
for (i = 0; i < cmem->nr_ranges; i++) {
ei.size = cmem->ranges[i].end - cmem->ranges[i].start + 1;
>> The image->elf_load_addr is determined by arch_kexec_locate_mem_hole(), this
>> function can ensure that the value is within the range of [crashk_res.start,
>> crashk_res.end), but it seems that it cannot guarantee that its value will
>> always be equal to crashk_res.start. Perhaps I have some omissions, please
>> point them out.
> Because elfcorehdr is the first one and only one that allocates memory from the
> starting address of crashk_res, and the starting address of crashk_res meets
> the alignment requirement of elfcorehdr.
>
> elfcorehdr requires 4k alignment, and the starting address of crashk_res is
> 16M aligned.
>
> Therefore, image->elf_load_addr should be equal to crashk_res.start.
Yes! you read the code very carefully and I didn't notice that! However, the
location of elfheader in crashk_res.start is highly dependent on elfheader in
crashk_res memory allocation order and position. At present, x86 first allocate
the memory of elfheader. However, ppc64 doesn't seem to be like this (It first
executes load_backup_segment()). Although arm64 allocates elfheader first, it
sets kbuf.top_down to true in load_other_segments(). This will cause the
elfheader to be allocated near crashk_res.end. I debugged using crash on the
arm64 machine and the result is(Although the kernel version of the testing
machine may be a bit low, the process of allocating elfheaders is consistent
with upstream):
crash> p crashk_res.start
$6 = 1375731712
crash> p crashk_res.end
$7 = 2147483647
crash> p kexec_crash_image.arch.elf_headers_mem
$9 = 2147352576
So I think it's best to set cmem->max_nr_ranges to 2 for easy maintenance in
the future. What do you think about ?