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

[Ma, Jingbai (Kingboard)]

On 9/18/13 10:17 PM, "Dave Anderson" <anderson at redhat.com> wrote:


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

Yes, I was want to do this, but now I would make it easier, just like you
said,
add a new 64-bit max_mapnr into the kdump_sub_header.

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

OK, this approach seems better and less risks. I will prepare patches for
makedumpfile and crash utility soon.
Thanks!

>
>Dave

--
Thanks,
Jingbai Ma