From: Dave Anderson <anderson@xxxxxxxxxx>
Subject: Re: [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump)
Date: Mon, 20 Aug 2012 15:47:16 -0400
> ----- Original Message -----
>> At 2012-8-20 16:32, qiaonuohan wrote:
>> >
>> > I have modified the patches, and they are based on crash 6.0.9.
>>
>> I find some mistakes in my former patches, please ignore them and refer
>> to the attachments of this mail.
>>
>> --
>> --
>> Regards
>> Qiao Nuohan
>
> The patch is looking better, but a few issues still remain to be
> cleaned up.
>
> I'm wondering about the correctness of this addition to read_netdump()
> for 32-bit dumpfiles:
>
> int
> read_netdump(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
> {
> off_t offset;
> struct pt_load_segment *pls;
> int i;
>
> + if (nd->flags & QEMU_MEM_DUMP_KDUMP_BACKUP &&
> + paddr >= nd->backup_src_start &&
> + paddr < nd->backup_src_start + nd->backup_src_size) {
> + ulong orig_paddr;
> +
> + orig_paddr = paddr;
> + paddr += nd->backup_offset - nd->backup_src_start;
> +
> + if (CRASHDEBUG(1))
> + error(INFO,
> + "qemu_mem_dump: kdump backup region: %#lx => %#lx\n",
> + orig_paddr, paddr);
> + }
>
> The incoming "paddr" parameter is type physaddr_t, which is declared as:
>
> typedef uint64_t physaddr_t;
>
> I see that you've pretty much copied the "if" statement from read_sadump(),
> but I'm not sure whether SADUMP supports 32-bit dumpfiles?
>
SADUMP supports 32-bit dumpfiles, so this is a bug. Thanks for
pointing out this.
> Since nd->backup_src_start is a physical address, maybe it should be
> declared as a physaddr_t as well? Or if the incoming paddr is 4GB or
> greater, then perhaps it shouldn't be checked against nd->backup_src_start.
> In other words, I'm not sure what happens when you do this:
>
> paddr >= nd->backup_src_start
>
> When running on a 32-bit machine, does paddr get truncated to a 32-bit value,
> or does nd->backup_src_start get promoted to a 64-bit value?
>
At least next test case on RHEL5.8 32-bit machines shows truncation in
32-bit direction.
#include <stdio.h>
#include <stdint.h>
int main(int argc, char **argv)
{
uint64_t u = (1ULL << 32);
unsigned long i = (unsigned long)-1;
if (u >= i)
printf("%#llx >= %#x\n", u, i);
else
printf("not %#llx >= %#x\n", u, i);
if (u < i)
printf("%#llx < %#x\n", u, i);
else
printf("not %#llx < %#x\n", u, i);
return 0;
}
[hat@vm-x86 test]$ gcc ./test.c -o test; ./test
not 0x100000000 >= 0xffffffff
0x100000000 < 0xffffffff
Please apply the attached patch.
Thanks.
HATAYAMA, Daisuke
>From 85477b05e728224fd786941b387eea21cdc219ce Mon Sep 17 00:00:00 2001
From: HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxxxxx>
Date: Tue, 21 Aug 2012 10:09:54 +0900
Subject: [PATCH] sadump: Fix invalid truncation of physical address to 32-bit values
There is invalid truncation in conversion of read requests into
back-up region where 32-bit or larger physical address is
truncated to 32-bit values, leading to unexpected result.
This patch fixes this by redefining the type of the address of
back-up region as 64-bit unsigned integer to avoid the
truncation.
Signed-off-by: HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxxxxx>
---
sadump.c | 7 ++++---
sadump.h | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/sadump.c b/sadump.c
index 795b4f8..f15d6de 100644
--- a/sadump.c
+++ b/sadump.c
@@ -1057,7 +1057,7 @@ int sadump_memory_dump(FILE *fp)
fprintf(fp, " block_table: %lx\n", (ulong)sd->block_table);
fprintf(fp, " sd_list_len: %d\n", sd->sd_list_len);
fprintf(fp, " sd_list: %lx\n", (ulong)sd->sd_list);
- fprintf(fp, " backup_src_start: %lx\n", sd->backup_src_start);
+ fprintf(fp, " backup_src_start: %llx\n", sd->backup_src_start);
fprintf(fp, " backup_src_size: %lx\n", sd->backup_src_size);
fprintf(fp, " backup_offset: %llx\n", (ulonglong)sd->backup_src_size);
@@ -1621,7 +1621,8 @@ void sadump_kdump_backup_region_init(void)
Elf64_Off e_phoff;
uint16_t e_phnum, e_phentsize;
uint64_t backup_offset;
- ulong backup_src_start, backup_src_size;
+ ulonglong backup_src_start;
+ ulong backup_src_size;
int kimage_segment_len;
size_t bufsize;
@@ -1715,7 +1716,7 @@ void sadump_kdump_backup_region_init(void)
if (CRASHDEBUG(1))
error(INFO,
"sadump: kexec backup region found: "
- "START: %#016lx SIZE: %#016lx OFFSET: %#016llx\n",
+ "START: %#016llx SIZE: %#016lx OFFSET: %#016llx\n",
backup_src_start, backup_src_size, backup_offset);
break;
diff --git a/sadump.h b/sadump.h
index 64c2630..29dce06 100644
--- a/sadump.h
+++ b/sadump.h
@@ -204,7 +204,7 @@ struct sadump_data {
/* Backup Region, First 640K of System RAM. */
#define KEXEC_BACKUP_SRC_END 0x0009ffff
- ulong backup_src_start;
+ ulonglong backup_src_start;
ulong backup_src_size;
ulonglong backup_offset;
};
--
1.7.7.6
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
