[PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[zijun_hu]

[-- Attachment #1.1: Type: text/plain, Size: 1266 bytes --]

Hi All,
         There is a bug in mm/memblock.c
         Could you review and phase-in this patch?
         Thanks a lot

From 3abf1822d30f77f126bd7a3c09bb243d9c17a029 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH] mm/memblock.c: fix index adjustment error in
__next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
mm/memblock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..b14973e 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1024,7 +1024,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
                                 *out_end = m_end;
                        if (out_nid)
                                 *out_nid = m_nid;
-                         idx_a++;
+                        idx_a--;
                        *idx = (u32)idx_a | (u64)idx_b << 32;
                        return;
               }
--
1.9.1


Zijun Hu
Tel: +86 021 3813 0008 Ext: 8556

F1 Building, 299 Kang Wei Road, Pudong New Area,
Shanghai 201315, China
htc.com



[-- Attachment #1.2: Type: text/html, Size: 9586 bytes --]

[-- Attachment #2: 0001-mm-memblock.c-fix-index-adjustment-error-in.patch --]
[-- Type: application/octet-stream, Size: 806 bytes --]

From 3abf1822d30f77f126bd7a3c09bb243d9c17a029 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..b14973e 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1024,7 +1024,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1

Re: [PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[Tejun Heo]

On Mon, Jul 25, 2016 at 07:34:12AM +0000, zijun_hu@htc.com wrote:
> Hi All,
>          There is a bug in mm/memblock.c
>          Could you review and phase-in this patch?
>          Thanks a lot
> 
> From 3abf1822d30f77f126bd7a3c09bb243d9c17a029 Mon Sep 17 00:00:00 2001
> From: zijun_hu <zijun_hu@htc.com>
> Date: Mon, 25 Jul 2016 15:06:57 +0800
> Subject: [PATCH] mm/memblock.c: fix index adjustment error in
> __next_mem_range_rev()
> 
> fix region index adjustment error when parameter type_b of
> __next_mem_range_rev() == NULL
> 
> Signed-off-by: zijun_hu <zijun_hu@htc.com>
> ---
> mm/memblock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index ac12489..b14973e 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1024,7 +1024,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
>                                  *out_end = m_end;
>                         if (out_nid)
>                                  *out_nid = m_nid;
> -                         idx_a++;
> +                        idx_a--;

Looks good to me.  Do you happen to have a test case for this bug?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[zijun_hu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset="gb2312", Size: 3515 bytes --]

Hi Tejun,

I am sorry, I don't take any test for the patch attached in previous mail, and it can't fix the bug completely, please ignore it
I provide a new patch attached in this mail which pass test and can fix the issue described below

__next_mem_range_rev() defined in mm/memblock.c doesn't Achieve desired purpose if parameter type_b ==NULL
This new patch can fix the issue and get the last reversed region contained in type_a rightly

The new patch is descripted as follows

From 0e242eda7696f176a9a2e585a1db01f0575b39c9 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..cc5aeab 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,11 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		/* in order to get the last reversed region rightly */
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1028,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1

Zijun Hu
Tel: +86 021 3813 0008 Ext: 8556

F1 Building, 299 Kang Wei Road, Pudong New Area,
Shanghai 201315, China
htc.com



-----ÓʼþÔ­¼þ-----
·¢¼þÈË: Tejun Heo [mailto:htejun@gmail.com] ´ú±í Tejun Heo
·¢ËÍʱ¼ä: 2016Äê7ÔÂ26ÈÕ 2:52
ÊÕ¼þÈË: Zijun Hu(ºú×ÔÜŠ)
³­ËÍ: akpm@linux-foundation.org; kuleshovmail@gmail.com; ard.biesheuvel@linaro.org; tangchen@cn.fujitsu.com; weiyang@linux.vnet.ibm.com; dev@g0hl1n.net; david@gibson.dropbear.id.au; linux-mm@kvack.org; linux-kernel@vger.kernel.org
Ö÷Ìâ: Re: [PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

On Mon, Jul 25, 2016 at 07:34:12AM +0000, zijun_hu@htc.com wrote:
> Hi All,
>          There is a bug in mm/memblock.c
>          Could you review and phase-in this patch?
>          Thanks a lot
> 
> From 3abf1822d30f77f126bd7a3c09bb243d9c17a029 Mon Sep 17 00:00:00 2001
> From: zijun_hu <zijun_hu@htc.com>
> Date: Mon, 25 Jul 2016 15:06:57 +0800
> Subject: [PATCH] mm/memblock.c: fix index adjustment error in
> __next_mem_range_rev()
> 
> fix region index adjustment error when parameter type_b of
> __next_mem_range_rev() == NULL
> 
> Signed-off-by: zijun_hu <zijun_hu@htc.com>
> ---
> mm/memblock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c index ac12489..b14973e 
> 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1024,7 +1024,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
>                                  *out_end = m_end;
>                         if (out_nid)
>                                  *out_nid = m_nid;
> -                         idx_a++;
> +                        idx_a--;

Looks good to me.  Do you happen to have a test case for this bug?

Thanks.

--
tejun


[-- Attachment #1.2: Type: text/html, Size: 6572 bytes --]

[-- Attachment #2: 0001-mm-memblock.c-fix-index-adjustment-error-in.patch --]
[-- Type: application/octet-stream, Size: 1163 bytes --]

From 0e242eda7696f176a9a2e585a1db01f0575b39c9 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..cc5aeab 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,11 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		/* in order to get the last reversed region rightly */
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1028,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1

Re: [PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[Tejun Heo]

Hello,

On Tue, Jul 26, 2016 at 03:03:58PM +0000, zijun_hu@htc.com wrote:
> I am sorry, I don't take any test for the patch attached in previous
> mail, and it can't fix the bug completely, please ignore it I
> provide a new patch attached in this mail which pass test and can
> fix the issue described below
>
> __next_mem_range_rev() defined in mm/memblock.c doesn't Achieve
> desired purpose if parameter type_b ==NULL This new patch can fix
> the issue and get the last reversed region contained in type_a
> rightly

Can you please flow future mails to 80 column?

> The new patch is descripted as follows
> 
> From 0e242eda7696f176a9a2e585a1db01f0575b39c9 Mon Sep 17 00:00:00 2001
> From: zijun_hu <zijun_hu@htc.com>
> Date: Mon, 25 Jul 2016 15:06:57 +0800
> Subject: [PATCH] mm/memblock.c: fix index adjustment error in
>  __next_mem_range_rev()
> 
> fix region index adjustment error when parameter type_b of
> __next_mem_range_rev() == NULL

The patch is now fixing two bugs.  It'd be nice to describe each in
the description and how the patch was tested.

> @@ -991,7 +991,11 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
>  
>  	if (*idx == (u64)ULLONG_MAX) {
>  		idx_a = type_a->cnt - 1;
> -		idx_b = type_b->cnt;
> +		/* in order to get the last reversed region rightly */

Before, it would trigger null deref.  I don't think the above comment
is necessary.

> +		if (type_b != NULL)
> +			idx_b = type_b->cnt;
> +		else
> +			idx_b = 0;
>  	}
>  
>  	for (; idx_a >= 0; idx_a--) {
> @@ -1024,7 +1028,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
>  				*out_end = m_end;
>  			if (out_nid)
>  				*out_nid = m_nid;
> -			idx_a++;
> +			idx_a--;
>  			*idx = (u32)idx_a | (u64)idx_b << 32;
>  			return;
>  		}

Both changes look good to me.  Provided the changes are tested,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[zijun_hu]

[-- Attachment #1.1: Type: text/plain, Size: 3951 bytes --]

Hi Tejun,
I find that there is a bug in __next_mem_range_rev() defined in mm/memblock.c

patch 0001 can fix the issue and pass test successfully, please help to review
and phase-in it
patch 0002 is used to verify the solution only and is provided for explaining
test method, please don't apply it

for __next_mem_range_rev(), it don't iterate through memory regions contained
in type_a in reversed order rightly if its parameter type_b == NULL
moreover, it will cause mass error loops if macro for_each_mem_range_rev is
called with parameter type_b == NULL

the patch 0001 corrects region index idx_a adjustment and initialize idx_b
to 0 to promise getting the last reversed region correctly if parameter
type_b == NULL as showed below

my test method is simple, namely, dump all types of regions with right kernel
interface and fixed __next_mem_range separately ,then check whether
fixed__next_mem_range achieve desired purpose, see test patch segments
below or entire patch 0002 for more info

thanks for Tejun's guidance and helping

fix patch 0001 is showed as follows

From da2f3cafab9632d59261cf0801f62e909d0bfde1 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH 1/2] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..e95f95f 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,10 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1027,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1


Test patch 002 code segments is showed as follows

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d45f862..0db80bb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -326,6 +326,13 @@ void __init bootmem_init(void)
 
 	high_memory = __va((max << PAGE_SHIFT) - 1) + 1;
 	memblock_dump_all();
+
+	if (!memblock_debug)
+		__memblock_dump_all();
+	/*
+	 * extern void memblock_patch_verify(void);
+	 */
+	memblock_patch_verify();
 }

diff --git a/mm/memblock.c b/mm/memblock.c
index e95f95f..5c179ae 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1652,6 +1652,31 @@ void __init_memblock __memblock_dump_all(void)
 	memblock_dump(&memblock.reserved, "reserved");
 }

+void __init_memblock memblock_patch_verify(void)
+{
+	u64 i;
+	phys_addr_t this_start, this_end;
+
+	pr_info("in %s: memory\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.reserved, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: memory X reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, &memblock.reserved,
+			NUMA_NO_NODE, MEMBLOCK_NONE,
+			&this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+}

Zijun Hu
F1 Building, 299 Kang Wei Road, Pudong New Area,
Shanghai 201315, China
htc.com









[-- Attachment #1.2: Type: text/html, Size: 6159 bytes --]

[-- Attachment #2: 0001-mm-memblock.c-fix-index-adjustment-error-in.patch --]
[-- Type: application/octet-stream, Size: 1108 bytes --]

From da2f3cafab9632d59261cf0801f62e909d0bfde1 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH 1/2] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..e95f95f 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,10 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1027,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1


[-- Attachment #3: 0002-mm-temporary-patch-for-fix-memblock-issue-test.patch --]
[-- Type: application/octet-stream, Size: 2502 bytes --]

From df753d7d9426b4d2a5518958d281be2985ccd40d Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Wed, 27 Jul 2016 12:13:37 +0800
Subject: [PATCH 2/2] mm: temporary patch for fix memblock issue test

temporary patch for fix memblock issue test

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 arch/arm64/mm/init.c     |  7 +++++++
 include/linux/memblock.h |  1 +
 mm/memblock.c            | 25 +++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d45f862..0db80bb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -326,6 +326,13 @@ void __init bootmem_init(void)
 
 	high_memory = __va((max << PAGE_SHIFT) - 1) + 1;
 	memblock_dump_all();
+
+	if (!memblock_debug)
+		__memblock_dump_all();
+	/*
+	 * extern void memblock_patch_verify(void);
+	 */
+	memblock_patch_verify();
 }
 
 #ifndef CONFIG_SPARSEMEM_VMEMMAP
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 3106ac1..c62df1e 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -340,6 +340,7 @@ bool memblock_is_reserved(phys_addr_t addr);
 bool memblock_is_region_reserved(phys_addr_t base, phys_addr_t size);
 
 extern void __memblock_dump_all(void);
+extern void memblock_patch_verify(void);
 
 static inline void memblock_dump_all(void)
 {
diff --git a/mm/memblock.c b/mm/memblock.c
index e95f95f..5c179ae 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1652,6 +1652,31 @@ void __init_memblock __memblock_dump_all(void)
 	memblock_dump(&memblock.reserved, "reserved");
 }
 
+void __init_memblock memblock_patch_verify(void)
+{
+	u64 i;
+	phys_addr_t this_start, this_end;
+
+	pr_info("in %s: memory\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.reserved, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: memory X reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, &memblock.reserved,
+			NUMA_NO_NODE, MEMBLOCK_NONE,
+			&this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+}
+
 void __init memblock_allow_resize(void)
 {
 	memblock_can_resize = 1;
-- 
1.9.1

[PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[zijun_hu]

[-- Attachment #1.1: Type: text/plain, Size: 3999 bytes --]

Hi Tejun,
I find that there is a bug in __next_mem_range_rev() defined in mm/memblock.c

patch 0001 can fix the issue and pass test successfully, please help to review
and phase-in it
patch 0002 is used to verify the solution only and is provided for explaining
test method, please don't apply it

for __next_mem_range_rev(), it not only triggers null deref issue but also
doesn't iterate through memory regions contained in type_a in reversed
order rightly if its parameter type_b == NULL,moreover, it will cause mass
error loops if macro for_each_mem_range_rev is called with parameter
type_b == NULL

the patch 0001 corrects region index idx_a adjustment and initialize idx_b
to 0 to promise getting the last reversed region correctly if parameter
type_b == NULL as showed below

my test method is simple, namely, dump all types of regions with right kernel
interface and fixed __next_mem_range separately ,then check whether
fixed__next_mem_range achieves desired purpose, see test patch
segments below or entire patch 0002 for more info

thanks for Tejun's guidance and helping

fix patch 0001 is showed as follows

From da2f3cafab9632d59261cf0801f62e909d0bfde1 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH 1/2] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..e95f95f 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,10 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1027,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1


Test patch 002 code segments is showed as follows

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d45f862..0db80bb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -326,6 +326,13 @@ void __init bootmem_init(void)
 
 	high_memory = __va((max << PAGE_SHIFT) - 1) + 1;
 	memblock_dump_all();
+
+	if (!memblock_debug)
+		__memblock_dump_all();
+	/*
+	 * extern void memblock_patch_verify(void);
+	 */
+	memblock_patch_verify();
 }

diff --git a/mm/memblock.c b/mm/memblock.c
index e95f95f..5c179ae 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1652,6 +1652,31 @@ void __init_memblock __memblock_dump_all(void)
 	memblock_dump(&memblock.reserved, "reserved");
 }

+void __init_memblock memblock_patch_verify(void)
+{
+	u64 i;
+	phys_addr_t this_start, this_end;
+
+	pr_info("in %s: memory\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.reserved, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: memory X reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, &memblock.reserved,
+			NUMA_NO_NODE, MEMBLOCK_NONE,
+			&this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+}

Zijun Hu
F1 Building, 299 Kang Wei Road, Pudong New Area,
Shanghai 201315, China
htc.com









[-- Attachment #1.2: Type: text/html, Size: 6231 bytes --]

[-- Attachment #2: 0001-mm-memblock.c-fix-index-adjustment-error-in.patch --]
[-- Type: application/octet-stream, Size: 1108 bytes --]

From da2f3cafab9632d59261cf0801f62e909d0bfde1 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Mon, 25 Jul 2016 15:06:57 +0800
Subject: [PATCH 1/2] mm/memblock.c: fix index adjustment error in
 __next_mem_range_rev()

fix region index adjustment error when parameter type_b of
__next_mem_range_rev() == NULL

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 mm/memblock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index ac12489..e95f95f 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -991,7 +991,10 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 
 	if (*idx == (u64)ULLONG_MAX) {
 		idx_a = type_a->cnt - 1;
-		idx_b = type_b->cnt;
+		if (type_b != NULL)
+			idx_b = type_b->cnt;
+		else
+			idx_b = 0;
 	}
 
 	for (; idx_a >= 0; idx_a--) {
@@ -1024,7 +1027,7 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 				*out_end = m_end;
 			if (out_nid)
 				*out_nid = m_nid;
-			idx_a++;
+			idx_a--;
 			*idx = (u32)idx_a | (u64)idx_b << 32;
 			return;
 		}
-- 
1.9.1


[-- Attachment #3: 0002-mm-temporary-patch-for-fix-memblock-issue-test.patch --]
[-- Type: application/octet-stream, Size: 2502 bytes --]

From df753d7d9426b4d2a5518958d281be2985ccd40d Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Wed, 27 Jul 2016 12:13:37 +0800
Subject: [PATCH 2/2] mm: temporary patch for fix memblock issue test

temporary patch for fix memblock issue test

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 arch/arm64/mm/init.c     |  7 +++++++
 include/linux/memblock.h |  1 +
 mm/memblock.c            | 25 +++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d45f862..0db80bb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -326,6 +326,13 @@ void __init bootmem_init(void)
 
 	high_memory = __va((max << PAGE_SHIFT) - 1) + 1;
 	memblock_dump_all();
+
+	if (!memblock_debug)
+		__memblock_dump_all();
+	/*
+	 * extern void memblock_patch_verify(void);
+	 */
+	memblock_patch_verify();
 }
 
 #ifndef CONFIG_SPARSEMEM_VMEMMAP
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 3106ac1..c62df1e 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -340,6 +340,7 @@ bool memblock_is_reserved(phys_addr_t addr);
 bool memblock_is_region_reserved(phys_addr_t base, phys_addr_t size);
 
 extern void __memblock_dump_all(void);
+extern void memblock_patch_verify(void);
 
 static inline void memblock_dump_all(void)
 {
diff --git a/mm/memblock.c b/mm/memblock.c
index e95f95f..5c179ae 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1652,6 +1652,31 @@ void __init_memblock __memblock_dump_all(void)
 	memblock_dump(&memblock.reserved, "reserved");
 }
 
+void __init_memblock memblock_patch_verify(void)
+{
+	u64 i;
+	phys_addr_t this_start, this_end;
+
+	pr_info("in %s: memory\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.reserved, NULL, NUMA_NO_NODE,
+			MEMBLOCK_NONE, &this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+
+	pr_info("in %s: memory X reserved\n", __func__);
+	for_each_mem_range_rev(i, &memblock.memory, &memblock.reserved,
+			NUMA_NO_NODE, MEMBLOCK_NONE,
+			&this_start, &this_end, NULL)
+		pr_info("[%#016llx]\t[%#016llx-%#016llx]\n",
+				i, this_start, this_end);
+}
+
 void __init memblock_allow_resize(void)
 {
 	memblock_can_resize = 1;
-- 
1.9.1

Re: [PATCH] mm/memblock.c: fix index adjustment error in __next_mem_range_rev()

[Tejun Heo]

Hello,

On Wed, Jul 27, 2016 at 06:49:40AM +0000, zijun_hu@htc.com wrote:
> patch 0001 can fix the issue and pass test successfully, please help to review
> and phase-in it
> patch 0002 is used to verify the solution only and is provided for explaining
> test method, please don't apply it

Great.

> for __next_mem_range_rev(), it don't iterate through memory regions contained
> in type_a in reversed order rightly if its parameter type_b == NULL
> moreover, it will cause mass error loops if macro for_each_mem_range_rev is
> called with parameter type_b == NULL
> 
> the patch 0001 corrects region index idx_a adjustment and initialize idx_b
> to 0 to promise getting the last reversed region correctly if parameter
> type_b == NULL as showed below
> 
> my test method is simple, namely, dump all types of regions with right kernel
> interface and fixed __next_mem_range separately ,then check whether
> fixed__next_mem_range achieve desired purpose, see test patch segments
> below or entire patch 0002 for more info

It'd be better to include how you tested in the patch description.

> fix patch 0001 is showed as follows
> 
> From da2f3cafab9632d59261cf0801f62e909d0bfde1 Mon Sep 17 00:00:00 2001
> From: zijun_hu <zijun_hu@htc.com>
> Date: Mon, 25 Jul 2016 15:06:57 +0800
> Subject: [PATCH 1/2] mm/memblock.c: fix index adjustment error in
>  __next_mem_range_rev()
> 
> fix region index adjustment error when parameter type_b of
> __next_mem_range_rev() == NULL
> 
> Signed-off-by: zijun_hu <zijun_hu@htc.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>