memblock: Make sure reserved.regions is freed really

memblock_free() would double reserved.regions too, so we could free old range for reserved.regions.

So need to check that regions get doubled. If it is doubled, we need to free it.

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 |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/memblock.c
===================================================================

mm/memblock.c +8 -2

@@ -148,11 +148,26 @@ phys_addr_t __init_memblock memblock_fin
  */
 int __init_memblock memblock_free_reserved_regions(void)
 {
+	struct memblock_region *r;
+	int ret;
+
 	if (memblock.reserved.regions == memblock_reserved_init_regions)
 		return 0;
 
-	return memblock_free(__pa(memblock.reserved.regions),
-		 sizeof(struct memblock_region) * memblock.reserved.max);
+	/*
+	 * During memblock_free, reserved.regions could be doubled,
+	 * try to check with old one for checking, and need to free the new one.
+	 */
+	do {

Hello, Yinghai.

Quoted 42 lines · Yinghai Lu

memblock_free() would double reserved.regions too, so we could free old range for reserved.regions.

So need to check that regions get doubled. If it is doubled, we need to free it.

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 |   23 +++++++++++++++++++++--

 1 file changed, 21 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/memblock.c
===================================================================

mm/memblock.c +14 -2

@@ -148,11 +148,26 @@ phys_addr_t __init_memblock memblock_fin
   */
  int __init_memblock memblock_free_reserved_regions(void)
  {
+     struct memblock_region *r;
+     int ret;
+
       if (memblock.reserved.regions == memblock_reserved_init_regions)
               return 0;
 
-     return memblock_free(__pa(memblock.reserved.regions),
-              sizeof(struct memblock_region) * memblock.reserved.max);
+     /*
+      * During memblock_free, reserved.regions could be doubled,
+      * try to check with old one for checking, and need to free the new one.
+      */
+     do {
+             r = memblock.reserved.regions;
+             ret = memblock_free(__pa(memblock.reserved.regions),
+                     sizeof(struct memblock_region) * memblock.reserved.max);
+
+             if (ret)
+                     break;

mm/memblock.c phys_addr_t __init_memblock memblock_fin · line 171

Quoted 4 lines

mm/memblock.c +3 -0

+     } while (memblock.reserved.regions != r);
+
+     return ret;
  }

Hmmm... nice catch but I think it's a bit complex and ugly.  In this case, we *know* that the region isn't gonna be split.  Maybe a better option is to add something like the following?

void memblock_remove_region_by_ptr(struct memblock_type *type, struct memblock_region *r) {
       WARN_ON(/* make sure @r is inside @type->regions */); memblock_remove_region(type, r - type->regions); }

Quoted 3 lines

Hmmm... nice catch but I think it's a bit complex and ugly.  In this case, we *know* that the region isn't gonna be split.  Maybe a better option is to add something like the following?

how do you know __pa(memblock.reserved.regions) would not be merged with other entries
in reserved.regions?

Quoted 1 line

Quoted 6 lines

void memblock_remove_region_by_ptr(struct memblock_type *type, struct memblock_region *r) {
       WARN_ON(/* make sure @r is inside @type->regions */); memblock_remove_region(type, r - type->regions); }

you want to change

Hey, Yinghai.

Quoted 9 lines · Yinghai Lu

Quoted 3 lines

Hmmm... nice catch but I think it's a bit complex and ugly.  In this case, we *know* that the region isn't gonna be split.  Maybe a better option is to add something like the following?

how do you know __pa(memblock.reserved.regions) would not be merged with other entries
in reserved.regions?

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

void memblock_remove_region_by_ptr(struct memblock_type *type, struct memblock_region *r) {
       WARN_ON(/* make sure @r is inside @type->regions */); memblock_remove_region(type, r - type->regions); }

you want to change

Heh, you're right.  My suggestion was completely bonkers.  Sorry about that.  Hmm.... how about separating out removal pre-expansion and doing it before calling into free?  ie. something like

static int memblock_prepare_for_isolation(struct memblock_type *type) {
       /* we'll create at most two more regions */
       while (type->cnt + 2 > type->max)
               if (memblock_double_array(type) < 0)
                       return -ENOMEM;
       return 0;
}

int __init_memblock memblock_free_reserved_regions(void) {
       int ret;

       if (memblock.reserved.regions == memblock_reserved_init_regions) return 0;

       ret = memblock_prepare_for_isolation(&memblock.reserved); if (ret)
               return ret;

       ret = memblock_free(__pa(memblock.reserved.regions), sizeof(struct memblock_region) * memblock.reserved.max); WARN_ON(ret || memblock.reserved.regions has changed); return ret;
}