[PATCH 0/2] cache: fix cache logic not go into invalid state

[PATCH 0/2] cache: fix cache logic not go into invalid state

[HATAYAMA Daisuke]

I faced failure of sadump phys_base calculation and found a bug in
cache.c causes it. Due to the bug, cache_alloc() returns NULL forever
throughout execution. The fix is the 2nd patch. During the
investigation I also found a lack of malloc() allocation failure
check. The fix is the 1st patch. Primary is the 2nd one.

I built this patch set on top of devel branch.

---

HATAYAMA Daisuke (2):
      cache: allocate buffers at initialization to detect malloc() failure
      cache: reuse entry in pending list


 cache.c        |   50 +++++++++++++++++++++++++++++++++-----------------
 cache.h        |    1 +
 makedumpfile.c |    3 +++
 3 files changed, 37 insertions(+), 17 deletions(-)

-- 

Thanks.
HATAYAMA, Daisuke

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/2] cache: allocate buffers at initialization to detect malloc() failure

[HATAYAMA Daisuke]

malloc() is used in cache_alloc() but there's no check for it. If I
added check in cache_alloc() directly, cache_alloc() needs to return
one more error status and code gets somewhat complicated. Instead, I
move malloc() in initial() to detect allocation failure at
initialization. By this change, 8 buffers are allocated at the same
time, no longer incrementally. However, 8 buffers are almost always
used throughout execution. There's essential differnece from the
incremental one.

Signed-off-by: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
---
 cache.c        |   29 ++++++++++++++++++++++-------
 cache.h        |    1 +
 makedumpfile.c |    3 +++
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/cache.c b/cache.c
index 3bea089..dad8d80 100644
--- a/cache.c
+++ b/cache.c
@@ -18,6 +18,7 @@
 
 #include "makedumpfile.h"
 #include "cache.h"
+#include "print_info.h"
 
 struct cache_entry {
 	unsigned long long paddr;
@@ -36,6 +37,25 @@ static int avail = CACHE_SIZE;
 
 static struct cache used, pending;
 
+int
+cache_init(void)
+{
+	void *bufptr;
+	int i;
+
+	for (i = 0; i < CACHE_SIZE; ++i) {
+		bufptr = malloc(info->page_size);
+		if (bufptr == NULL) {
+			ERRMSG("Can't allocate memory for cache. %s\n",
+			       strerror(errno));
+			return FALSE;
+		}
+		pool[i].bufptr = bufptr;
+	}
+
+	return TRUE;
+}
+
 static void
 add_entry(struct cache *cache, struct cache_entry *entry)
 {
@@ -83,13 +103,8 @@ cache_alloc(unsigned long long paddr)
 {
 	struct cache_entry *entry = NULL;
 
-	if (avail) {
-		void *bufptr = malloc(info->page_size);
-		if (bufptr) {
-			entry = &pool[--avail];
-			entry->bufptr = bufptr;
-		}
-	}
+	if (avail)
+		entry = &pool[--avail];
 
 	if (!entry) {
 		if (used.tail) {
diff --git a/cache.h b/cache.h
index f37d883..4730e12 100644
--- a/cache.h
+++ b/cache.h
@@ -19,6 +19,7 @@
 #ifndef _CACHE_H
 #define _CACHE_H
 
+int cache_init(void);
 void *cache_search(unsigned long long paddr);
 void *cache_alloc(unsigned long long paddr);
 void cache_add(unsigned long long paddr);
diff --git a/makedumpfile.c b/makedumpfile.c
index 1718f88..e01ff50 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3017,6 +3017,9 @@ out:
 		DEBUG_MSG("Buffer size for the cyclic mode: %ld\n", info->bufsize_cyclic);
 	}
 
+	if (!cache_init())
+		return FALSE;
+
 	if (debug_info) {
 		if (info->flag_sadump)
 			(void) sadump_virt_phys_base();


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/2] cache: reuse entry in pending list

[HATAYAMA Daisuke]

Currently, sadump_virt_phys_base() fails to calculate phys_base as
below:

$ /pub/repos/makedumpfile/makedumpfile -f -p -d 31 -x /pub3/vmcores/comparevmcore_data/vmlinux-2.6.18-363.el5 /pub3/vmcores/comparevmcore_data/vmcore-2.6.18-363.el5-sadump /pub3/vmcores/comparevmcore_data/vmcore-pd31
Switched running mode from cyclic to non-cyclic,
because the cyclic mode doesn't support sadump format.
readmem: type_addr: 1, addr:296020, size:13
readmem: type_addr: 1, addr:ffffffffff296020, size:13
readmem: type_addr: 1, addr:ffffffffff396020, size:13
readmem: type_addr: 1, addr:ffffffffff496020, size:13
readmem: type_addr: 1, addr:ffffffffff596020, size:13
readmem: type_addr: 1, addr:ffffffffff696020, size:13
readmem: type_addr: 1, addr:ffffffffff796020, size:13
readmem: type_addr: 1, addr:ffffffffff896020, size:13
readmem: type_addr: 1, addr:ffffffffff996020, size:13
readmem: type_addr: 1, addr:ffffffffffa96020, size:13
readmem: type_addr: 1, addr:ffffffffffb96020, size:13
readmem: type_addr: 1, addr:ffffffffffc96020, size:13
readmem: type_addr: 1, addr:ffffffffffd96020, size:13
readmem: type_addr: 1, addr:ffffffffffe96020, size:13
readmem: type_addr: 1, addr:fffffffffff96020, size:13
readmem: type_addr: 1, addr:96020, size:13
readmem: type_addr: 1, addr:196020, size:13
readmem: type_addr: 1, addr:296020, size:13
readmem: type_addr: 1, addr:396020, size:13
readmem: type_addr: 1, addr:496020, size:13
readmem: type_addr: 1, addr:596020, size:13
readmem: type_addr: 1, addr:696020, size:13
readmem: type_addr: 1, addr:796020, size:13
readmem: type_addr: 1, addr:896020, size:13
readmem: type_addr: 1, addr:996020, size:13
readmem: type_addr: 1, addr:a96020, size:13
readmem: type_addr: 1, addr:b96020, size:13
readmem: type_addr: 1, addr:c96020, size:13
readmem: type_addr: 1, addr:d96020, size:13
readmem: type_addr: 1, addr:e96020, size:13
readmem: type_addr: 1, addr:f96020, size:13
readmem: type_addr: 1, addr:1096020, size:13
readmem: type_addr: 1, addr:1196020, size:13
readmem: type_addr: 1, addr:1296020, size:13
readmem: type_addr: 0, addr:ffffffff8045e260, size:32
cpu_online_mask_init: Can't read cpu_online_mask memory.

makedumpfile Failed.

By git bisect, I found this bug is caused by the following commit:

commit 0aff0e5174d0708bf1bfb039ab863e1fea8a1029
Author: Petr Tesarik <ptesarik@suse.cz>
Date:   Wed Oct 31 16:12:47 2012 +0900

    [PATCH] keep dumpfile pages in a cache.

Then, I found this bug happens in the following senario.

If one of the readpage_xxx() methods fails reading 8 pages in a row, 8
entries in pool are fully contained in pending list. Then,
cache_alloc() returns NULL and this continues forever in the
execution. In other words, there's assumption in cache_alloc() that if
pool is fully used, they are fully in used list, not in pending list
at all. However, the buggy path here breaks the assumption. This patch
changes cache_alloc() so that it first tries to reuse enty in pending
list if exists.

In fact, I found this bug in ad-hoc phys_base calculation performed in
sadump_virt_phys_base(). However, I fixed cache side since this bug
can occur in general on every vmcore format. Crash dump can contain
broken data in any part of vmcore and so requested physical address to
read can be broken likewise.

Signed-off-by: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
---
 cache.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/cache.c b/cache.c
index dad8d80..0dd957c 100644
--- a/cache.c
+++ b/cache.c
@@ -103,19 +103,20 @@ cache_alloc(unsigned long long paddr)
 {
 	struct cache_entry *entry = NULL;
 
-	if (avail)
+	if (avail) {
 		entry = &pool[--avail];
-
-	if (!entry) {
-		if (used.tail) {
-			entry = used.tail;
-			remove_entry(&used, entry);
-		} else
-			return NULL;
-	}
-
-	entry->paddr = paddr;
-	add_entry(&pending, entry);
+		entry->paddr = paddr;
+		add_entry(&pending, entry);
+	} else if (pending.tail) {
+		entry = pending.tail;
+		entry->paddr = paddr;
+	} else if (used.tail) {
+		entry = used.tail;
+		remove_entry(&used, entry);
+		entry->paddr = paddr;
+		add_entry(&pending, entry);
+	} else
+		return NULL;
 
 	return entry->bufptr;
 }


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/2] cache: fix cache logic not go into invalid state

[HATAYAMA Daisuke]

Hello Kumagai-san,

Could you review these patches?

(2013/09/17 15:29), HATAYAMA Daisuke wrote:
> I faced failure of sadump phys_base calculation and found a bug in
> cache.c causes it. Due to the bug, cache_alloc() returns NULL forever
> throughout execution. The fix is the 2nd patch. During the
> investigation I also found a lack of malloc() allocation failure
> check. The fix is the 1st patch. Primary is the 2nd one.
>
> I built this patch set on top of devel branch.
>
> ---
>
> HATAYAMA Daisuke (2):
>        cache: allocate buffers at initialization to detect malloc() failure
>        cache: reuse entry in pending list
>
>
>   cache.c        |   50 +++++++++++++++++++++++++++++++++-----------------
>   cache.h        |    1 +
>   makedumpfile.c |    3 +++
>   3 files changed, 37 insertions(+), 17 deletions(-)
>


-- 
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/2] cache: fix cache logic not go into invalid state

[Atsushi Kumagai]

Hello HATAYAMA-san,

(2013/09/19 9:54), HATAYAMA Daisuke wrote:
> Hello Kumagai-san,
>
> Could you review these patches?

Good catch, thanks for your fixing.
I'll merge this patch set into v1.5.5.

BTW, I'm going to be on vacation from Sep 21 until Sep 30,
I can't reply for a while.


Thanks
Atsushi Kumagai

>
> (2013/09/17 15:29), HATAYAMA Daisuke wrote:
>> I faced failure of sadump phys_base calculation and found a bug in
>> cache.c causes it. Due to the bug, cache_alloc() returns NULL forever
>> throughout execution. The fix is the 2nd patch. During the
>> investigation I also found a lack of malloc() allocation failure
>> check. The fix is the 1st patch. Primary is the 2nd one.
>>
>> I built this patch set on top of devel branch.
>>
>> ---
>>
>> HATAYAMA Daisuke (2):
>>        cache: allocate buffers at initialization to detect malloc() failure
>>        cache: reuse entry in pending list
>>
>>
>>   cache.c        |   50 +++++++++++++++++++++++++++++++++-----------------
>>   cache.h        |    1 +
>>   makedumpfile.c |    3 +++
>>   3 files changed, 37 insertions(+), 17 deletions(-)
>>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec