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

[Sergio Lopez]

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.

> > 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.
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?

> >  		} 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?

Sergio.