[BUG] [compressed kdump / SADUMP] makedumpfile header truncation error
Ma, Jingbai (Kingboard)
kingboard.ma at hp.com
Thu Sep 19 08:55:54 EDT 2013
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?
>
>> > 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
>
>
>
>
More information about the kexec
mailing list