The __alloc_memory_core_early() routine will ask memblock for a range
of memory then try to reserve it. If the reserved region array lacks
space for the new range, memblock_double_array() is called to allocate
more space for the array. If memblock is used to allocate memory for
the new array it can end up using a range that overlaps with the range
originally allocated in __alloc_memory_core_early(), leading to possible
data corruption.
With this patch memblock_double_array() now calls memblock_find_in_range()
with a narrowed candidate range (in cases where the reserved.regions array
is being doubled) so any memory allocated will not overlap with the original
range that was being reserved. The range is narrowed by passing in the
starting address and size of the previously allocated range. Then the
range above the ending address is searched and if a candidate is not
found, the range below the starting address is searched.
Changes from v1 to v2 (based on comments from Yinghai Lu):
- use obase instead of base in memblock_add_region() for excluding start address
- pass in both the starting and ending address of the exclude range to
memblock_double_array()
- have memblock_double_array() search above the exclude ending address
and below the exclude starting address for a free range
Changes from v2 to v3 (based on comments from Yinghai Lu):
- pass in exclude_start and exclude_size to memblock_double_array()
- only exclude a range if doubling the reserved.regions array
- make sure narrowed range passed to memblock_find_in_range() is accessible
- to make the code less confusing, change memblock_isolate_range() to
pass in exclude_start and exclude_size
- remove unneeded comment in memblock_add_region() between while and
one line loop body
Changes from v3 to v4 (based on comments from Tejun Heo):
- change parameter names passed to memblock_double_array() so they
are not misleading and better signify the reason why the array is
being doubled
- add function comment block to memblock_double_arry() to ensure
the details of the possible overlap are explained
Signed-off-by: Greg Pearson <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
---
mm/memblock.c | 36 ++++++++++++++++++++++++++++++++----
1 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 952123e..0e737d9 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -184,7 +184,24 @@ static void __init_memblock memblock_remove_region(struct memblock_type *type, u
}
}
-static int __init_memblock memblock_double_array(struct memblock_type *type)
+/**
+ * memblock_double_array - double the size of the memblock regions array
+ * @type: memblock type of the regions array being doubled
+ * @new_area_start: starting address of memory range to avoid overlap with
+ * @new_area_size: size of memory range to avoid overlap with
+ *
+ * Double the size of the @type regions array. If memblock is being used to
+ * allocate memory for a new reserved regions array and there is a previously
+ * allocated memory range [@new_area_start,@new_area_start+@new_area_size]
+ * waiting to be reserved, ensure the memory used by the new array does
+ * not overlap.
+ *
+ * RETURNS:
+ * 0 on success, -1 on failure.
+ */
+static int __init_memblock memblock_double_array(struct memblock_type *type,
+ phys_addr_t new_area_start,
+ phys_addr_t new_area_size)
{
struct memblock_region *new_array, *old_array;
phys_addr_t old_size, new_size, addr;
@@ -222,7 +239,18 @@ static int __init_memblock memblock_double_array(struct memblock_type *type)
new_array = kmalloc(new_size, GFP_KERNEL);
addr = new_array ? __pa(new_array) : 0;
} else {
- addr = memblock_find_in_range(0, MEMBLOCK_ALLOC_ACCESSIBLE, new_size, sizeof(phys_addr_t));
+ /* only exclude range when trying to double reserved.regions */
+ if (type != &memblock.reserved)
+ new_area_start = new_area_size = 0;
+
+ addr = memblock_find_in_range(new_area_start + new_area_size,
+ memblock.current_limit,
+ new_size, sizeof(phys_addr_t));
+ if (!addr && new_area_size)
+ addr = memblock_find_in_range(0,
+ min(new_area_start, memblock.current_limit),
+ new_size, sizeof(phys_addr_t));
+
new_array = addr ? __va(addr) : 0;
}
if (!addr) {
@@ -399,7 +427,7 @@ repeat:
*/
if (!insert) {
while (type->cnt + nr_new > type->max)
- if (memblock_double_array(type) < 0)
+ if (memblock_double_array(type, obase, size) < 0)
return -ENOMEM;
insert = true;
goto repeat;
@@ -450,7 +478,7 @@ static int __init_memblock memblock_isolate_range(struct memblock_type *type,
/* we'll create at most two more regions */
while (type->cnt + 2 > type->max)
- if (memblock_double_array(type) < 0)
+ if (memblock_double_array(type, base, size) < 0)
return -ENOMEM;
for (i = 0; i < type->cnt; i++) {
--
1.7.5.4
On Mon, Jun 18, 2012 at 05:47:58PM -0600, Greg Pearson wrote:
> The __alloc_memory_core_early() routine will ask memblock for a range
> of memory then try to reserve it. If the reserved region array lacks
> space for the new range, memblock_double_array() is called to allocate
> more space for the array. If memblock is used to allocate memory for
> the new array it can end up using a range that overlaps with the range
> originally allocated in __alloc_memory_core_early(), leading to possible
> data corruption.
>
> With this patch memblock_double_array() now calls memblock_find_in_range()
> with a narrowed candidate range (in cases where the reserved.regions array
> is being doubled) so any memory allocated will not overlap with the original
> range that was being reserved. The range is narrowed by passing in the
> starting address and size of the previously allocated range. Then the
> range above the ending address is searched and if a candidate is not
> found, the range below the starting address is searched.
>
> Changes from v1 to v2 (based on comments from Yinghai Lu):
> - use obase instead of base in memblock_add_region() for excluding start address
> - pass in both the starting and ending address of the exclude range to
> memblock_double_array()
> - have memblock_double_array() search above the exclude ending address
> and below the exclude starting address for a free range
>
> Changes from v2 to v3 (based on comments from Yinghai Lu):
> - pass in exclude_start and exclude_size to memblock_double_array()
> - only exclude a range if doubling the reserved.regions array
> - make sure narrowed range passed to memblock_find_in_range() is accessible
> - to make the code less confusing, change memblock_isolate_range() to
> pass in exclude_start and exclude_size
> - remove unneeded comment in memblock_add_region() between while and
> one line loop body
>
> Changes from v3 to v4 (based on comments from Tejun Heo):
> - change parameter names passed to memblock_double_array() so they
> are not misleading and better signify the reason why the array is
> being doubled
> - add function comment block to memblock_double_arry() to ensure
> the details of the possible overlap are explained
>
> Signed-off-by: Greg Pearson <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Yinghai Lu <[email protected]>
Acked-by: Tejun Heo <[email protected]>
The SOB tag from Yinghai is a bit weird tho. SOB indicates the chain
of custody for the patch, so the above SOBs indicate that the patch is
originally from Greg and then routed (rolled into series or branch) by
Yinghai which isn't the case here. I suppose it's either Reviewed-by:
or Acked-by:?
Thanks.
--
tejun
On 06/19/2012 03:33 PM, Tejun Heo wrote:
> On Mon, Jun 18, 2012 at 05:47:58PM -0600, Greg Pearson wrote:
>> The __alloc_memory_core_early() routine will ask memblock for a range
>> of memory then try to reserve it. If the reserved region array lacks
>> space for the new range, memblock_double_array() is called to allocate
>> more space for the array. If memblock is used to allocate memory for
>> the new array it can end up using a range that overlaps with the range
>> originally allocated in __alloc_memory_core_early(), leading to possible
>> data corruption.
>>
>> With this patch memblock_double_array() now calls memblock_find_in_range()
>> with a narrowed candidate range (in cases where the reserved.regions array
>> is being doubled) so any memory allocated will not overlap with the original
>> range that was being reserved. The range is narrowed by passing in the
>> starting address and size of the previously allocated range. Then the
>> range above the ending address is searched and if a candidate is not
>> found, the range below the starting address is searched.
>>
>> Changes from v1 to v2 (based on comments from Yinghai Lu):
>> - use obase instead of base in memblock_add_region() for excluding start address
>> - pass in both the starting and ending address of the exclude range to
>> memblock_double_array()
>> - have memblock_double_array() search above the exclude ending address
>> and below the exclude starting address for a free range
>>
>> Changes from v2 to v3 (based on comments from Yinghai Lu):
>> - pass in exclude_start and exclude_size to memblock_double_array()
>> - only exclude a range if doubling the reserved.regions array
>> - make sure narrowed range passed to memblock_find_in_range() is accessible
>> - to make the code less confusing, change memblock_isolate_range() to
>> pass in exclude_start and exclude_size
>> - remove unneeded comment in memblock_add_region() between while and
>> one line loop body
>>
>> Changes from v3 to v4 (based on comments from Tejun Heo):
>> - change parameter names passed to memblock_double_array() so they
>> are not misleading and better signify the reason why the array is
>> being doubled
>> - add function comment block to memblock_double_arry() to ensure
>> the details of the possible overlap are explained
>>
>> Signed-off-by: Greg Pearson <[email protected]>
>> Cc: Tejun Heo <[email protected]>
>> Cc: Benjamin Herrenschmidt <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Signed-off-by: Yinghai Lu <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
>
> The SOB tag from Yinghai is a bit weird tho. SOB indicates the chain
> of custody for the patch, so the above SOBs indicate that the patch is
> originally from Greg and then routed (rolled into series or branch) by
> Yinghai which isn't the case here. I suppose it's either Reviewed-by:
> or Acked-by:?
>
> Thanks.
>
Tejun,
I wasn't quite sure what to do about that at first either, I read
"Documentation/SubmittingPatches" and it says:
"The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path."
Since Yinghai contributed some code that is in the current version of
the patch I thought the "Signed-off-by" tag would be ok, but if
something else is more appropriate I have no problem re-cutting the
patch to make the chain of custody more clear.
Thanks
--
Greg
On Mon, 18 Jun 2012 17:47:58 -0600
Greg Pearson <[email protected]> wrote:
> The __alloc_memory_core_early() routine will ask memblock for a range
> of memory then try to reserve it. If the reserved region array lacks
> space for the new range, memblock_double_array() is called to allocate
> more space for the array. If memblock is used to allocate memory for
> the new array it can end up using a range that overlaps with the range
> originally allocated in __alloc_memory_core_early(), leading to possible
> data corruption.
OK, but we have no information about whether it *does* lead to data
corruption. Are there workloads which trigger this? End users who are
experiencing problems?
See, I (and others) need to work out whether this patch should be
included in 3.5 or even earlier kernels. To do that we often need the
developer to tell us what the impact of the bug is upon users. Please
always include this info when fixing bugs.
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -399,7 +427,7 @@ repeat:
> */
> if (!insert) {
> while (type->cnt + nr_new > type->max)
> - if (memblock_double_array(type) < 0)
> + if (memblock_double_array(type, obase, size) < 0)
> return -ENOMEM;
> insert = true;
> goto repeat;
Minor nit: it would be nicer to make memblock_double_array() return 0
on success or a -ve errno, and then propagate that errno back. This is
more flexible than having the caller *assume* that the callee failed for a
particular reason.
On Tue, 19 Jun 2012 22:00:22 +0000
"Pearson, Greg" <[email protected]> wrote:
> I wasn't quite sure what to do about that at first either, I read
> "Documentation/SubmittingPatches" and it says:
>
> "The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path."
>
> Since Yinghai contributed some code that is in the current version of
> the patch I thought the "Signed-off-by" tag would be ok, but if
> something else is more appropriate I have no problem re-cutting the
> patch to make the chain of custody more clear.
Yup, we shouldn't expect people to be able to magically infer
fine-grained details such as this from hints embedded in the signoff
trail. Fortunately we can write stuff in English ;)
I added
: This patch contains contributions from Yinghai Lu.
to the changelog. Simple, huh? :)
On 06/19/2012 04:14 PM, Andrew Morton wrote:
> On Mon, 18 Jun 2012 17:47:58 -0600
> Greg Pearson <[email protected]> wrote:
>
>> The __alloc_memory_core_early() routine will ask memblock for a range
>> of memory then try to reserve it. If the reserved region array lacks
>> space for the new range, memblock_double_array() is called to allocate
>> more space for the array. If memblock is used to allocate memory for
>> the new array it can end up using a range that overlaps with the range
>> originally allocated in __alloc_memory_core_early(), leading to possible
>> data corruption.
> OK, but we have no information about whether it *does* lead to data
> corruption. Are there workloads which trigger this? End users who are
> experiencing problems?
>
> See, I (and others) need to work out whether this patch should be
> included in 3.5 or even earlier kernels. To do that we often need the
> developer to tell us what the impact of the bug is upon users. Please
> always include this info when fixing bugs.
Andrew,
I'm currently working on a prototype system that exhibits the data
corruption problem when doubling the reserved array while booting the
system. This system will be a released product in the future. I'll
remember to include this information in the patch next time.
Thanks
--
Greg-
On Tue, 19 Jun 2012 22:35:08 +0000
"Pearson, Greg" <[email protected]> wrote:
> On 06/19/2012 04:14 PM, Andrew Morton wrote:
> > On Mon, 18 Jun 2012 17:47:58 -0600
> > Greg Pearson <[email protected]> wrote:
> >
> >> The __alloc_memory_core_early() routine will ask memblock for a range
> >> of memory then try to reserve it. If the reserved region array lacks
> >> space for the new range, memblock_double_array() is called to allocate
> >> more space for the array. If memblock is used to allocate memory for
> >> the new array it can end up using a range that overlaps with the range
> >> originally allocated in __alloc_memory_core_early(), leading to possible
> >> data corruption.
> > OK, but we have no information about whether it *does* lead to data
> > corruption. Are there workloads which trigger this? End users who are
> > experiencing problems?
> >
> > See, I (and others) need to work out whether this patch should be
> > included in 3.5 or even earlier kernels. To do that we often need the
> > developer to tell us what the impact of the bug is upon users. Please
> > always include this info when fixing bugs.
>
> Andrew,
>
> I'm currently working on a prototype system that exhibits the data
> corruption problem when doubling the reserved array while booting the
> system. This system will be a released product in the future.
OK. I guess we can slip this fix into 3.5. Do you think it should be
backported? I guess "yes", as you will probably want to run 3.4 or
earlier kernels on that machine.
On 06/19/2012 05:00 PM, Andrew Morton wrote:
> On Tue, 19 Jun 2012 22:35:08 +0000
> "Pearson, Greg" <[email protected]> wrote:
>
>> On 06/19/2012 04:14 PM, Andrew Morton wrote:
>>> On Mon, 18 Jun 2012 17:47:58 -0600
>>> Greg Pearson <[email protected]> wrote:
>>>
>>>> The __alloc_memory_core_early() routine will ask memblock for a range
>>>> of memory then try to reserve it. If the reserved region array lacks
>>>> space for the new range, memblock_double_array() is called to allocate
>>>> more space for the array. If memblock is used to allocate memory for
>>>> the new array it can end up using a range that overlaps with the range
>>>> originally allocated in __alloc_memory_core_early(), leading to possible
>>>> data corruption.
>>> OK, but we have no information about whether it *does* lead to data
>>> corruption. Are there workloads which trigger this? End users who are
>>> experiencing problems?
>>>
>>> See, I (and others) need to work out whether this patch should be
>>> included in 3.5 or even earlier kernels. To do that we often need the
>>> developer to tell us what the impact of the bug is upon users. Please
>>> always include this info when fixing bugs.
>> Andrew,
>>
>> I'm currently working on a prototype system that exhibits the data
>> corruption problem when doubling the reserved array while booting the
>> system. This system will be a released product in the future.
> OK. I guess we can slip this fix into 3.5. Do you think it should be
> backported? I guess "yes", as you will probably want to run 3.4 or
> earlier kernels on that machine.
>
Having the fix in 3.4 would be good for us, as we do plan to test on the
latest stable kernel.
If there is anything I can do to help with that please let me know.
Thanks
--
Greg-