Re: [BUG] [compressed kdump / SADUMP] makedumpfile header truncation error

Dave Anderson
2013-09-18T14:18:19Z

----- Original Message -----
> On 09/17/2013 09:23 PM, Dave Anderson wrote:
> >
> >
> > ----- Original Message -----
> >> On 09/17/2013 03:33 PM, HATAYAMA Daisuke wrote:
> >>> (2013/09/17 16:12), Jingbai Ma wrote:
> >>>> On 09/17/2013 02:55 PM, HATAYAMA Daisuke wrote:
> >>>>
> >>>> int32_t, int64_t, uint64_t, etc ... are parts of C99 standard:
> >>>> http://en.wikipedia.org/wiki/C_data_types
> >>>> All there types have been supported by GCC, so them should work on all
> >>>> the architectures.
> >>>>
> >>>> Although change these persistent data structure will affect both
> >>>> makedumpfile and crash utility, but we will benefit from the
> >>>> consistent data structures independent from architectures. We can
> >>>> analyze a dumpfile on a OS with different architecture than the
> >>>> crashed OS.
> >>>>
> >>>>
> >>>
> >>> I know stdint.h things and usefulness if we can use crash and makedumpfile
> >>> for a multiple architectures on single arch. In fact, crash already supports
> >>> cross platform build among some architectures thanks to Dave.
> >
> > But only if the host and target architectures have the same endian-ness and
> > whose data type sizes match.
> >
>
> If we have a standard for the dump file format, we can handle all the endian-ness issues.
> I know it may affect the dumping speed on the platform that has to
> convert the byte order. But at least we can specify the byte order for
> dump file header. It won't cost too much.
>
> > The only problem that has ever been seen with the current header declarations
> > is if an x86 crash binary is built to support the 32-bit ARM architecture.
> > For x86, the 64-bit off_t variables can start on a 4-byte boundary, but on ARM,
> > they have to start on an 8-byte boundary. That being the case, the off_t
> > offset_vmcoreinfo is at offset 20 when built on an x86, and at offset 24
> > when built on ARM:
>
> This could be addressed through compiler attributes:
> off_t offset_vmcoreinfo __atttribute__ ((aligned(8));
> offset_vmcoreinfo will be the same 8-byte boundary on x86 as same as ARM
>
> >
> > struct kdump_sub_header {
> > unsigned long phys_base;
> > int dump_level; /* header_version 1 and later */
> > int split; /* header_version 2 and later */
> > unsigned long start_pfn; /* header_version 2 and later */
> > unsigned long end_pfn; /* header_version 2 and later */
> > off_t offset_vmcoreinfo; /* header_version 3 and later */
> > unsigned long size_vmcoreinfo; /* header_version 3 and later */
> > off_t offset_note; /* header_version 4 and later */
> > unsigned long size_note; /* header_version 4 and later */
> > off_t offset_eraseinfo; /* header_version 5 and later */
> > unsigned long size_eraseinfo; /* header_version 5 and later */
> > };
> >
>
> Do you like this change?
> struct kdump_sub_header {
> unsigned long phys_base;
> int dump_level;
> int split;
> unsigned long start_pfn;
> unsigned long end_pfn;
> off_t offset_vmcoreinfo __atttribute__ ((aligned(8));
> unsigned long size_vmcoreinfo;
> off_t offset_note __atttribute__ ((aligned(8));
> unsigned long size_note;
> off_t offset_eraseinfo __atttribute__ ((aligned(8));
> unsigned long size_eraseinfo;
> };
>
> Then you can get rid of the padded struct kdump_sub_header_ARM_target in
> crash utility.

Adding the aligned(8) attribute to the kdump_sub_header would break
compatibility with all of the old/current 32-bit x86 dumpfiles that
have it aligned on an 4-byte boundary. How do you propose working
around that?

>
> Or we can go further, redefine whole structure and set all fields with
> specific bit width.
>
> struct kdump_sub_header {
> uint64_t phys_base;
> int32_t dump_level;
> int32_t split;
> uint64_t start_pfn;
> uint64_t end_pfn;
> uint64_t offset_vmcoreinfo;
> uint64_t size_vmcoreinfo;
> uint64_t offset_note;
> uint64_t size_note;
> uint64_t offset_eraseinfo;
> uint64_t size_eraseinfo;
> };
>
> I have checked the code of crash utility, it shouldn't affect too much,
> only in diskdump.c and diskdump.h.
>
> > So for that anomoly, crash has to support a kdump_sub_header_ARM_target
> > structure that has a pad integer after the end_pfn variable.
> >
> >>>
> >>> My question came from the fact that it looks like you introduced a single
> >>> modified kdump_sub_header structure for all the architectures. They might
> >>> have different combination of length between int and long and maybe
> >>> also have other each architecture specific incompatibility. It wouldn't
> >>> work well.
> >>>
> >>> But from your reply, I think you mean a fully new header for kdump-compressed
> >>> format, right? If so, it must work well. But of course you need to modify
> >>> both of makedumpfile and crash utility to support it.
> >>>
> >>
> >> Yes, I would like to have a new header for kdump-compressed format. But
> >> I'm not sure how much code will be affected in makedumpfile and crash utility.
> >> I'm still under investigating, any ideas would be appreciated.
> >
> > The challenging part will be the requirement to maintain backwards-compatibility,
> > at least in the crash utility. And backwards-compatibility would also be required
> > in makedumpfile, right? For example, if you want to re-filter an older compressed
> > kdump.
> >
>
> It's not a big deal, we can check the header_version to decide treat it
> as traditional format or new format.
> We can preserve the current structures as kdump_sub_header_v5 ,
> kdump_sub_header_v5, etc... in both makedumpfile and crash utility.

OK, but I still don't see how to avoid carrying two versions of kdump_sub_header_v5
to handle the current ARM-on-x86 support. Or doing some kind of similar kludge...

Supporting both a kdump_sub_header and a kdump_sub_header_v5 is going to make
read_dump_header() a bit tricky. I suppose after reading the raw kdump_sub_header
block (of either type), if it's v5 or less you could copy the individual fields
from the kdump_sub_header_v5 to the new kdump_sub_header before referencing them?

> > But if -- as has been done so far -- an increment of the header_version in the
> > disk_dump_header to signal an additional field in the kdump_sub_header would be
> > trivial to implement.
>
> Yes, this approach is more simpler, but the drawback is we have to add a
> new 64bit max_mapnr_64 to disk_dump_header, then we will have two
> max_mapnr* fields, not very nice. And when we add more platforms, we
> still have to take care of the bit width and alignment.
> Should we fix it in this version or just leave it as it used to be?

Note that I suggested above that the 64-bit max_mapnr be added to
the kdump_sub_header, so that the disk_dump_header itself can remain
backwards-compatible.

Dave