Hi Takao,
A couple things. Can you post against the current github sources,
or at least use crash-7.2.0? I'm not sure what version you're
working with:
$ patch -p1 --dry-run < $dl/sadump_1.patch
checking file defs.h
Hunk #1 succeeded at 5629 (offset 7 lines).
checking file x86_64.c
Hunk #1 FAILED at 119.
Hunk #2 succeeded at 1998 (offset 34 lines).
Hunk #3 succeeded at 2016 (offset 34 lines).
Hunk #4 succeeded at 2030 (offset 34 lines).
Hunk #5 succeeded at 2086 (offset 34 lines).
1 out of 5 hunks FAILED
$ patch -p1 --dry-run < $dl/sadump_2.patch
checking file defs.h
Hunk #1 succeeded at 2590 (offset 5 lines).
Hunk #2 succeeded at 6312 (offset 47 lines).
checking file sadump.c
checking file sadump.h
checking file symbols.c
Hunk #3 succeeded at 12262 (offset 10 lines).
$
It was easy enough to address the FAILED segment above,
but there are a few issues with the sadump.c patch:
$ make warn
... [ cut ] ...
cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 sadump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security
sadump.c:1911:1: warning: no previous prototype for ‘get_kaslr_offset_from_vmcoreinfo’ [-Wmissing-prototypes]
get_kaslr_offset_from_vmcoreinfo(ulong cr3, ulong orig_kaslr_offset,
^
sadump.c: In function ‘sadump_calc_kaslr_offset’:
sadump.c:2088:38: warning: ‘scs.Cr3’ may be used uninitialized in this function [-Wmaybe-uninitialized]
if (get_kaslr_offset_from_vmcoreinfo(
^
sadump.c:2044:40: warning: ‘scs.IdtLower’ may be used uninitialized in this function [-Wmaybe-uninitialized]
idtr = ((uint64_t)scs.IdtUpper)<<32 | (uint64_t)scs.IdtLower;
^
sadump.c:2044:10: warning: ‘scs.IdtUpper’ may be used uninitialized in this function [-Wmaybe-uninitialized]
idtr = ((uint64_t)scs.IdtUpper)<<32 | (uint64_t)scs.IdtLower;
...
But more importantly, the sadump.c patch won't compile against the
other architectures, because x86_64_kvtop_pagetable() only exists
when crash is built for x86_64.
Plus I don't particularly like the kvtop "pagetable" argument abstraction
and new functions that you put in place in x86_64.c just for these 3 rather
arcane calls from sadump.c.
I suggest adding a new machdep->flags bit (USE_PT?) which could
be temporarily set in sadump.c prior to each of the 3 calls that
could be made to the generic kvtop() function. Then in x86_64_kvtop(),
check for the new machdep->flag as opposed to the "use_pagetable" argument.
It may even be possible to use SADUMP() along with some other existing
flag instead of creating a new one. But please just avoid creating the
new functions in x86_64.c.
Thanks,
Dave
----- Original Message -----
> Hi Dave, Hatayama-san,
>
> Hatayama-san, thanks for your review, I updated may patch.
>
> These patch series fix a problem that crash cannot open a dumpfile which is
> captured by sadump on KASLR enabled kernel.
>
> When KASLR feature is enabled, a kernel is placed on the memory randomly and
> therefore crash cannot open a dumpfile because addresses of kernel symbols in
> vmlinux are different from actual addresses. In the case of kdump,
> information
> to get actual address is included in the vmcoreinfo, but dumpfile of sadump
> does
> not have such a information.
>
> These patches calculate kaslr offset and phys_base to solve this problem. The
> basic idea is getting register (IDTR and CR3) from dump header, and calculate
> kaslr_offset/phys_base using them.
>
> changelog:
> v2:
> - Remove virsh-dump part
> - Change get_vec0_addr style
> - Some tiny fixes
>
> v1:
> https://www.redhat.com/archives/crash-utility/2017-October/msg00004.html
>
> Takao Indoh (2):
> Introduce x86_64_kvtop_pagetable
> Fix a KASLR problem of sadump
>
> defs.h | 5 +
> sadump.c | 458
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> sadump.h | 1 +
> symbols.c | 34 +++++
> x86_64.c | 26 +++-
> 5 files changed, 522 insertions(+), 2 deletions(-)
>
> --
> 2.9.5
>
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
