[PATCH] makedumpfile: wrong cyclic buffer size recalculation causes bitmap data corruption

[Baoquan He]

Description of Problem:

In cyclic mode, makedumpfile recalculates cyclic buffer size as the
largest multiple of the largest block size managed by buddy
allocator, i.e. 4MB, smaller than the cyclic buffer size in order to
enable to process each unit of blocks managed by buddy allocator in
each cycle.

However, makedumpfile does two wrong things in the recalculations:

1) While updating size of cyclic buffer, makedumpfile doesn't update
length of range of cycle in page frame numbers, due to which, if
cyclic buffer size is updated, because cyclic buffer size is always
reduced during udpate, some buffer overrun can happen on the cyclic
buffer. This can cause segmentation violation in the worst case.

2) roundup() is used to calculate bitmap size for maximum block size
managed by buddy allocator, here divideup() is correct, due to
which, although memory filtering is not affected, cyclic buffer size
get too much aligned and less efficient.

Fix patches has already been posted and merged in makedumpfile
development devel branch.

git://git.code.sf.net/p/makedumpfile/code
f8c8218856effc43ea01cd9394761cfb8aeaa8df
a785fa7dd7a7bd7dcbb017d0bea8848243b0924f

Signed-off-by: Baoquan He 
---
 ...e-pfn_cyclic-when-the-cyclic-buffer-size-.patch | 34 ++++++++++++++++
 ...ivideup-to-calculate-maximum-required-bit.patch | 45 ++++++++++++++++++++++
 kexec-tools.spec                                   |  4 ++
 3 files changed, 83 insertions(+)
 create mode 100644 kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
 create mode 100644 kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch

diff --git a/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
new file mode 100644
index 0000000..dc5bde4
--- /dev/null
+++ b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
@@ -0,0 +1,34 @@
+From a785fa7dd7a7bd7dcbb017d0bea8848243b0924f Mon Sep 17 00:00:00 2001
+Message-Id: 
+From: Atsushi Kumagai 
+Date: Thu, 12 Sep 2013 08:31:28 +0900
+Subject: [PATCH 1/2] [PATCH] Update pfn_cyclic when the cyclic buffer size is
+ corrected.
+
+When the clearing bit operation for excluding free pages can overrun
+the cyclic buffer, the buffer size is changed with
+check_cyclic_buffer_overrun().
+Then pfn_cyclic should be recalculated.
+
+Reviewed-by: HATAYAMA Daisuke 
+Signed-off-by: Atsushi Kumagai 
+Signed-off-by: Baoquan He 
+---
+ makedumpfile.c | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
+index 09c0d4a..164b3f1 100644
+--- a/makedumpfile-1.5.4/makedumpfile.c
++++ b/makedumpfile-1.5.4/makedumpfile.c
+@@ -4091,6 +4091,7 @@ check_cyclic_buffer_overrun(void)
+ 
+ 		bufsize = info->bufsize_cyclic;
+ 		info->bufsize_cyclic = round(bufsize, max_block_size);
++		info->pfn_cyclic = info->bufsize_cyclic * BITPERBYTE;
+ 
+ 		MSG("cyclic buffer size has been changed: %lu => %lu\n",
+ 		    bufsize, info->bufsize_cyclic);
+-- 
+1.8.3.1
+
diff --git a/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
new file mode 100644
index 0000000..e2e2b49
--- /dev/null
+++ b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
@@ -0,0 +1,45 @@
+From f8c8218856effc43ea01cd9394761cfb8aeaa8df Mon Sep 17 00:00:00 2001
+Message-Id: 
+In-Reply-To: 
+References: 
+From: HATAYAMA Daisuke 
+Date: Thu, 12 Sep 2013 13:19:00 +0900
+Subject: [PATCH 2/2] [PATCH] Use divideup() to calculate maximum required
+ bitmap size.
+
+Currently, check_cyclic_buffer_overrun() wrongly calculates maximum
+bitmap size required to represent maximum block size managed by buddy
+allocator with roundup(). Then, max_block_size is BITPERBYTE-time
+larger than its correct size. As a result, although the bug never
+affect free-page filtering since roundup(max_order_nr_pages,
+BITPERBYTE) is a multiple of divideup(max_order_nr_pages, BITPERBYTE),
+the following sanity check, (max_block_size > info->bufsize_cyclic),
+and recalculation of info->bufsize_cyclic becomes BITPERBYTE-time
+conservative and inefficient.
+
+Signed-off-by: HATAYAMA Daisuke 
+Signed-off-by: Baoquan He 
+---
+ makedumpfile.c | 5 ++---
+ 1 file changed, 2 insertions(+), 3 deletions(-)
+
+diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
+index 164b3f1..1718f88 100644
+--- a/makedumpfile-1.5.4/makedumpfile.c
++++ b/makedumpfile-1.5.4/makedumpfile.c
+@@ -4078,10 +4078,9 @@ check_cyclic_buffer_overrun(void)
+ {
+ 	int max_order = ARRAY_LENGTH(zone.free_area);
+ 	int max_order_nr_pages = 1 << (max_order - 1);
+-	unsigned long max_block_size = roundup(max_order_nr_pages, BITPERBYTE);
++	unsigned long max_block_size = divideup(max_order_nr_pages, BITPERBYTE);
+ 
+-	if (info->bufsize_cyclic %
+-	    roundup(max_order_nr_pages, BITPERBYTE)) {
++	if (info->bufsize_cyclic % max_block_size) {
+ 		unsigned long bufsize;
+ 
+ 		if (max_block_size > info->bufsize_cyclic) {
+-- 
+1.8.3.1
+
diff --git a/kexec-tools.spec b/kexec-tools.spec
index 513537a..b1b9e68 100644
--- a/kexec-tools.spec
+++ b/kexec-tools.spec
@@ -82,6 +82,8 @@ Patch301: kexec-tools-2.0.4-makedumpfile-Add-vmap_area_list-definition-for-ppc-p
 Patch601: kexec-tools-2.0.3-disable-kexec-test.patch
 Patch604: kexec-tools-2.0.3-build-makedumpfile-eppic-shared-object.patch
 Patch605: kexec-tools-2.0.4-makedumpfile-PATCH-Support-newer-kernels.patch
+Patch606: kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
+Patch607: kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
 Patch606: kexec-tools-2.0.4-makedumpfile-Fix-max_mapnr-issue-on-system-has-over-44-b.patch

%description
@@ -116,6 +118,8 @@ tar -z -x -v -f %{SOURCE19}
 %patch604 -p1
 %patch605 -p1
 %patch606 -p1
+%patch607 -p1
+%patch606 -p1
 %patch001 -p1
 %patch002 -p1
 %patch003 -p1

[WANG Chao]

On 10/22/13 at 05:07pm, Baoquan He wrote:
> Description of Problem:
>
> In cyclic mode, makedumpfile recalculates cyclic buffer size as the
> largest multiple of the largest block size managed by buddy
> allocator, i.e. 4MB, smaller than the cyclic buffer size in order to
> enable to process each unit of blocks managed by buddy allocator in
> each cycle.
>
> However, makedumpfile does two wrong things in the recalculations:
> While updating size of cyclic buffer, makedumpfile doesn't update
>
> length of range of cycle in page frame numbers, due to which, if
> cyclic buffer size is updated, because cyclic buffer size is always
> reduced during udpate, some buffer overrun can happen on the cyclic
> buffer. This can cause segmentation violation in the worst case.
> roundup() is used to calculate bitmap size for maximum block size
>
> managed by buddy allocator, here divideup() is correct, due to
> which, although memory filtering is not affected, cyclic buffer size
> get too much aligned and less efficient.
>
> Fix patches has already been posted and merged in makedumpfile
> development devel branch.
>
> git://git.code.sf.net/p/makedumpfile/code
> f8c8218856effc43ea01cd9394761cfb8aeaa8df
> a785fa7dd7a7bd7dcbb017d0bea8848243b0924f
>
> Signed-off-by: Baoquan He

Looks good to me.

Acked-by: WANG Chao
> ...e-pfn_cyclic-when-the-cyclic-buffer-size-.patch | 34 ++++++++++++++++
>  ...ivideup-to-calculate-maximum-required-bit.patch | 45 ++++++++++++++++++++++
>  kexec-tools.spec                                   |  4 ++
>  3 files changed, 83 insertions(+)
>  create mode 100644 kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
>  create mode 100644 kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
>
> diff --git a/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> new file mode 100644
> index 0000000..dc5bde4
> --- /dev/null
> +++ b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> @@ -0,0 +1,34 @@
> +From a785fa7dd7a7bd7dcbb017d0bea8848243b0924f Mon Sep 17 00:00:00 2001
> +Message-Id: 
> +From: Atsushi Kumagai 
> +Date: Thu, 12 Sep 2013 08:31:28 +0900
> +Subject: [PATCH 1/2] [PATCH] Update pfn_cyclic when the cyclic buffer size is
> corrected.
>
> +When the clearing bit operation for excluding free pages can overrun
> +the cyclic buffer, the buffer size is changed with
> +check_cyclic_buffer_overrun().
> +Then pfn_cyclic should be recalculated.
>
> +Reviewed-by: HATAYAMA Daisuke 
> +Signed-off-by: Atsushi Kumagai 
> +Signed-off-by: Baoquan He 
> +---
> makedumpfile.c | 1 +
> 1 file changed, 1 insertion(+)
>
> +diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
> +index 09c0d4a..164b3f1 100644
> +--- a/makedumpfile-1.5.4/makedumpfile.c
> ++++ b/makedumpfile-1.5.4/makedumpfile.c
> +@@ -4091,6 +4091,7 @@ check_cyclic_buffer_overrun(void)
> bufsize = info->bufsize_cyclic;
> info->bufsize_cyclic = round(bufsize, max_block_size);
>
> ++		info->pfn_cyclic = info->bufsize_cyclic * BITPERBYTE;
> MSG("cyclic buffer size has been changed: %lu => %lu\n",
>     bufsize, info->bufsize_cyclic);
>
> +-- 
> +1.8.3.1
>
> diff --git a/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
> new file mode 100644
> index 0000000..e2e2b49
> --- /dev/null
> +++ b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
> @@ -0,0 +1,45 @@
> +From f8c8218856effc43ea01cd9394761cfb8aeaa8df Mon Sep 17 00:00:00 2001
> +Message-Id: 
> +In-Reply-To: 
> +References: 
> +From: HATAYAMA Daisuke 
> +Date: Thu, 12 Sep 2013 13:19:00 +0900
> +Subject: [PATCH 2/2] [PATCH] Use divideup() to calculate maximum required
> bitmap size.
>
> +Currently, check_cyclic_buffer_overrun() wrongly calculates maximum
> +bitmap size required to represent maximum block size managed by buddy
> +allocator with roundup(). Then, max_block_size is BITPERBYTE-time
> +larger than its correct size. As a result, although the bug never
> +affect free-page filtering since roundup(max_order_nr_pages,
> +BITPERBYTE) is a multiple of divideup(max_order_nr_pages, BITPERBYTE),
> +the following sanity check, (max_block_size > info->bufsize_cyclic),
> +and recalculation of info->bufsize_cyclic becomes BITPERBYTE-time
> +conservative and inefficient.
>
> +Signed-off-by: HATAYAMA Daisuke 
> +Signed-off-by: Baoquan He 
> +---
> makedumpfile.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> +diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
> +index 164b3f1..1718f88 100644
> +--- a/makedumpfile-1.5.4/makedumpfile.c
> ++++ b/makedumpfile-1.5.4/makedumpfile.c
> +@@ -4078,10 +4078,9 @@ check_cyclic_buffer_overrun(void)
> {
> int max_order = ARRAY_LENGTH(zone.free_area);
> int max_order_nr_pages = 1 << (max_order - 1);
>
> +-	unsigned long max_block_size = roundup(max_order_nr_pages, BITPERBYTE);
> ++	unsigned long max_block_size = divideup(max_order_nr_pages, BITPERBYTE);
>
> +-	if (info->bufsize_cyclic %
> +-	    roundup(max_order_nr_pages, BITPERBYTE)) {
> ++	if (info->bufsize_cyclic % max_block_size) {
> unsigned long bufsize;
> if (max_block_size > info->bufsize_cyclic) {
>
> +-- 
> +1.8.3.1
>
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index 513537a..b1b9e68 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -82,6 +82,8 @@ Patch301: kexec-tools-2.0.4-makedumpfile-Add-vmap_area_list-definition-for-ppc-p
>  Patch601: kexec-tools-2.0.3-disable-kexec-test.patch
>  Patch604: kexec-tools-2.0.3-build-makedumpfile-eppic-shared-object.patch
>  Patch605: kexec-tools-2.0.4-makedumpfile-PATCH-Support-newer-kernels.patch
> +Patch606: kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> +Patch607: kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
>  Patch606: kexec-tools-2.0.4-makedumpfile-Fix-max_mapnr-issue-on-system-has-over-44-b.patch
>
> %description
> @@ -116,6 +118,8 @@ tar -z -x -v -f %{SOURCE19}
>  %patch604 -p1
>  %patch605 -p1
>  %patch606 -p1
> +%patch607 -p1
> +%patch606 -p1
>  %patch001 -p1
>  %patch002 -p1
>  %patch003 -p1
> -- 
> 1.8.3.1
>
> kexec mailing list
> [e-mail]
> https://lists.fedoraproject.org/mailman/listinfo/kexec

[Vivek Goyal]

On Tue, Oct 22, 2013 at 05:07:07PM +0800, Baoquan He wrote:
> Description of Problem:
>
> In cyclic mode, makedumpfile recalculates cyclic buffer size as the
> largest multiple of the largest block size managed by buddy
> allocator, i.e. 4MB, smaller than the cyclic buffer size in order to
> enable to process each unit of blocks managed by buddy allocator in
> each cycle.
>
> However, makedumpfile does two wrong things in the recalculations:
> While updating size of cyclic buffer, makedumpfile doesn't update
>
> length of range of cycle in page frame numbers, due to which, if
> cyclic buffer size is updated, because cyclic buffer size is always
> reduced during udpate, some buffer overrun can happen on the cyclic
> buffer. This can cause segmentation violation in the worst case.
> roundup() is used to calculate bitmap size for maximum block size
>
> managed by buddy allocator, here divideup() is correct, due to
> which, although memory filtering is not affected, cyclic buffer size
> get too much aligned and less efficient.
>
> Fix patches has already been posted and merged in makedumpfile
> development devel branch.
>
> git://git.code.sf.net/p/makedumpfile/code
> f8c8218856effc43ea01cd9394761cfb8aeaa8df
> a785fa7dd7a7bd7dcbb017d0bea8848243b0924f
>
> Signed-off-by: Baoquan He

Acked-by: Vivek Goyal

Vivek
> ...e-pfn_cyclic-when-the-cyclic-buffer-size-.patch | 34 ++++++++++++++++
>  ...ivideup-to-calculate-maximum-required-bit.patch | 45 ++++++++++++++++++++++
>  kexec-tools.spec                                   |  4 ++
>  3 files changed, 83 insertions(+)
>  create mode 100644 kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
>  create mode 100644 kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
>
> diff --git a/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> new file mode 100644
> index 0000000..dc5bde4
> --- /dev/null
> +++ b/kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> @@ -0,0 +1,34 @@
> +From a785fa7dd7a7bd7dcbb017d0bea8848243b0924f Mon Sep 17 00:00:00 2001
> +Message-Id: 
> +From: Atsushi Kumagai 
> +Date: Thu, 12 Sep 2013 08:31:28 +0900
> +Subject: [PATCH 1/2] [PATCH] Update pfn_cyclic when the cyclic buffer size is
> corrected.
>
> +When the clearing bit operation for excluding free pages can overrun
> +the cyclic buffer, the buffer size is changed with
> +check_cyclic_buffer_overrun().
> +Then pfn_cyclic should be recalculated.
>
> +Reviewed-by: HATAYAMA Daisuke 
> +Signed-off-by: Atsushi Kumagai 
> +Signed-off-by: Baoquan He 
> +---
> makedumpfile.c | 1 +
> 1 file changed, 1 insertion(+)
>
> +diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
> +index 09c0d4a..164b3f1 100644
> +--- a/makedumpfile-1.5.4/makedumpfile.c
> ++++ b/makedumpfile-1.5.4/makedumpfile.c
> +@@ -4091,6 +4091,7 @@ check_cyclic_buffer_overrun(void)
> bufsize = info->bufsize_cyclic;
> info->bufsize_cyclic = round(bufsize, max_block_size);
>
> ++		info->pfn_cyclic = info->bufsize_cyclic * BITPERBYTE;
> MSG("cyclic buffer size has been changed: %lu => %lu\n",
>     bufsize, info->bufsize_cyclic);
>
> +-- 
> +1.8.3.1
>
> diff --git a/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
> new file mode 100644
> index 0000000..e2e2b49
> --- /dev/null
> +++ b/kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
> @@ -0,0 +1,45 @@
> +From f8c8218856effc43ea01cd9394761cfb8aeaa8df Mon Sep 17 00:00:00 2001
> +Message-Id: 
> +In-Reply-To: 
> +References: 
> +From: HATAYAMA Daisuke 
> +Date: Thu, 12 Sep 2013 13:19:00 +0900
> +Subject: [PATCH 2/2] [PATCH] Use divideup() to calculate maximum required
> bitmap size.
>
> +Currently, check_cyclic_buffer_overrun() wrongly calculates maximum
> +bitmap size required to represent maximum block size managed by buddy
> +allocator with roundup(). Then, max_block_size is BITPERBYTE-time
> +larger than its correct size. As a result, although the bug never
> +affect free-page filtering since roundup(max_order_nr_pages,
> +BITPERBYTE) is a multiple of divideup(max_order_nr_pages, BITPERBYTE),
> +the following sanity check, (max_block_size > info->bufsize_cyclic),
> +and recalculation of info->bufsize_cyclic becomes BITPERBYTE-time
> +conservative and inefficient.
>
> +Signed-off-by: HATAYAMA Daisuke 
> +Signed-off-by: Baoquan He 
> +---
> makedumpfile.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> +diff --git a/makedumpfile-1.5.4/makedumpfile.c b/makedumpfile-1.5.4/makedumpfile.c
> +index 164b3f1..1718f88 100644
> +--- a/makedumpfile-1.5.4/makedumpfile.c
> ++++ b/makedumpfile-1.5.4/makedumpfile.c
> +@@ -4078,10 +4078,9 @@ check_cyclic_buffer_overrun(void)
> {
> int max_order = ARRAY_LENGTH(zone.free_area);
> int max_order_nr_pages = 1 << (max_order - 1);
>
> +-	unsigned long max_block_size = roundup(max_order_nr_pages, BITPERBYTE);
> ++	unsigned long max_block_size = divideup(max_order_nr_pages, BITPERBYTE);
>
> +-	if (info->bufsize_cyclic %
> +-	    roundup(max_order_nr_pages, BITPERBYTE)) {
> ++	if (info->bufsize_cyclic % max_block_size) {
> unsigned long bufsize;
> if (max_block_size > info->bufsize_cyclic) {
>
> +-- 
> +1.8.3.1
>
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index 513537a..b1b9e68 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -82,6 +82,8 @@ Patch301: kexec-tools-2.0.4-makedumpfile-Add-vmap_area_list-definition-for-ppc-p
>  Patch601: kexec-tools-2.0.3-disable-kexec-test.patch
>  Patch604: kexec-tools-2.0.3-build-makedumpfile-eppic-shared-object.patch
>  Patch605: kexec-tools-2.0.4-makedumpfile-PATCH-Support-newer-kernels.patch
> +Patch606: kexec-tools-2.0.4-makedumpfile-Update-pfn_cyclic-when-the-cyclic-buffer-size-.patch
> +Patch607: kexec-tools-2.0.4-makedumpfile-Use-divideup-to-calculate-maximum-required-bit.patch
>  Patch606: kexec-tools-2.0.4-makedumpfile-Fix-max_mapnr-issue-on-system-has-over-44-b.patch
>
> %description
> @@ -116,6 +118,8 @@ tar -z -x -v -f %{SOURCE19}
>  %patch604 -p1
>  %patch605 -p1
>  %patch606 -p1
> +%patch607 -p1
> +%patch606 -p1
>  %patch001 -p1
>  %patch002 -p1
>  %patch003 -p1
> -- 
> 1.8.3.1
>
> kexec mailing list
> [e-mail]
> https://lists.fedoraproject.org/mailman/listinfo/kexec