Re: [Crash-utility] [PATCH v2 0/4] Generalize KASLR calculation and use it for KDUMPs

----- Original Message ----- > On Fri, Mar 16, 2018 at 04:33:41PM -0400, Dave Anderson wrote: > > > > Hi Sergio, > > > > A few initial comments/questions/concerns about this patch... > > > > > diff --git a/diskdump.c b/diskdump.c > > > index b08a46c..1ec4bcf 100644 > > > --- a/diskdump.c > > > +++ b/diskdump.c > > > @@ -56,6 +56,7 @@ struct diskdump_data { > > > void **nt_prstatus_percpu; > > > uint num_prstatus_notes; > > > void **nt_qemu_percpu; > > > + void **nt_qemucs_percpu; > > > uint num_qemu_notes; > > > > > > /* page cache */ > > > @@ -72,6 +73,7 @@ struct diskdump_data { > > > ulong *valid_pages; > > > ulong accesses; > > > ulong snapshot_task; > > > + ulong kaslr_phys_base; > > > }; > > > > Generally speaking, there already is an sd->phys_base, and you've > > added an nd->phys_base, but I don't understand why you also added > > a new dd->kaslr_phys_base member and new diskdump_kaslr_phys_base() > > function? Is there any reason that you can't continue to use the > > currently-existing dd->sub_header_kdump->phys_base member and the > > diskdump_phys_base() function? I just find the concept of a > > "kaslr_phys_base" confusing, i.e., as if there are two different > > types of phys_base in the kernel. Can you please try to utilize > > the existing member and function? > > > > Also related, your diskdump_kaslr_phys_base() and kdump_phys_base() > > functions > > don't account for (return FALSE) with a legitimate phys_base value of 0. > > In fact I have a sample RHEL7 ELF vmcore generated by virsh dump which > > has a phys_base of 0. More on that below... > > Both of these are the consequence of trying to avoid changing the current > SADUMP's implementation while keeping NETDUMP's and DISKDUMP's as similar as > possible. > > But if there's no problem in changing SADUMP's, I would change the setters to > succeed unconditionally, and remove the "if (base->phys_base)" check from the > getters. This would make them semantically equivalent to current's > "diskdump_phys_base", which is the main reason I've added kaslr_phys_base, > allowing me to use the existing dd->sub_header_kdump->phys_base. No, I would leave the SADUMP implementation as it is. First, I don't maintain it, and secondly, apparently the hardware that it runs on always has a non-zero phys_base? I really don't know. The sadump maintainers will be ACK'ing this patchset as well, so I will leave it up to them. But diskdump_phys_base() and kdump_phys_base() -- as long as the dumpfile type is verified -- should return whatever is there, including zero, regardless whether it is the initialization value or set legitimately. The subsequent call to x86_64_virt_phys_base() will either verify it, or if lucky, calculate it += 16MB. Make sense?

> > > > diff --git a/kaslr_helper.c b/kaslr_helper.c > > > index 1079863..5b71e3e 100644 > > > --- a/kaslr_helper.c > > > +++ b/kaslr_helper.c > > > @@ -386,6 +386,9 @@ calc_kaslr_offset(ulong *kaslr_offset, ulong > > > *phys_base) > > > if (KDUMP_DUMPFILE()) { > > > idtr = kdump_get_idtr(); > > > cr3 = kdump_get_cr3(); > > > + } else if (DISKDUMP_DUMPFILE()) { > > > + idtr = diskdump_get_idtr(); > > > + cr3 = diskdump_get_cr3(); > > > > All 4 of these new functions above can fail and return 0. Probably > > unlikely, but shouldn't there be a FALSE return if either one is 0, > > rather than continuing and using them? > > Again, I was trying to keep in sync with current SADUMP's implementation. Yeah, you're right -- looking at the current sadump implementation, it looks like even though its get_sadump_smram_cpu_state_any() function can return FALSE, sadump_calc_kaslr_offset() doesn't bother to check it. > Otherwise, I'd prefer implementing a single function like this: > > int [diskdump|kdump|sadump]_get_idtr_cr3(uint64_t *idtr, uint64_t *cr3); > > That would allow me to write some like: > > if ((KDUMP_DUMPFILE() && !kdump_get_idtr_cr3(&idtr, &cr3)) || > (DISKDUMP_DUMPFILE() && !diskdump_get_idtr_cr3(&idtr, &cr3))) { > return FALSE; > } > > What do you think? Looks OK -- but since your kdump_get_idtr() is identical to diskdump_get_idtr(), and kdump_get_cr3() is identical to diskdump_get_cr3(), why can't the 4 functions be merged into a single function, and put in the new kaslr_helper.c? I understand why the two get_qemucpustate() functions exist.

> > > > } else { > > > return FALSE; > > > } > > > > > diff --git a/x86_64.c b/x86_64.c > > > index ed5985a..3c492e4 100644 > > > --- a/x86_64.c > > > +++ b/x86_64.c > > > @@ -6632,8 +6632,15 @@ x86_64_calc_phys_base(void) > > > * Get relocation value from whatever dumpfile format is being used. > > > */ > > > > > > - if (QEMU_MEM_DUMP_NO_VMCOREINFO() && KDUMP_DUMPFILE()) { > > > - if (kdump_phys_base(&phys_base)) { > > > + if (QEMU_MEM_DUMP_NO_VMCOREINFO()) { > > > + int ret; > > > + > > > + if (KDUMP_DUMPFILE()) > > > + ret = kdump_phys_base(&phys_base); > > > + else if (DISKDUMP_DUMPFILE()) > > > + ret = diskdump_kaslr_phys_base(&phys_base); > > > + > > > + if (ret) { > > > machdep->machspec->phys_base = phys_base; > > > if (CRASHDEBUG(1)) > > > fprintf(fp, "kdump-novmci: phys base: %lx\n", > > > -- > > > 2.14.3 > > > > This is where the "0 phys_base" issue comes into play. I think the section > > above should do the same thing as the following "if (DISKDUMP_DUMPFILE())" > > does, where diskump_phys_base() is only concerned if the dumpfile itself > > is valid. It may return 0 as a phys_base, and that's OK, because it > > unconditionally calls x86_64_virt_phys_base() -- which serves a dual > > purpose, either to: > > > > (1) verify it, or > > (2) if it's bogus, it checks whether plus-or-minus 16MB works. > > After switching to using dd->sub_header_kdump->phys_base, do you think we can > just leave QEMU_MEM_DUMP_COMPRESSED to be dealt by the DISKDUMP_DUMPFILE > section, and add another equivalent one for QEMU_MEM_DUMP_ELF && !VMCOREINFO? I would keep the QEMU_MEM_DUMP_NO_VMCOREINFO() section where it is, because it still potentially calls your new kdump_phys_base().