Hi Dave, Thanks for your comments. On Mon, Oct 16, 2017 at 04:56:01PM -0400, Dave Anderson wrote: > > 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, It seems that I made patches on old branch :-( I'll rebase them. > 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; > ... > Will fix them in next version. > 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. Ok, I'll add UST_PT or something and update patches. Thanks, Takao Indoh > > 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 -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility
