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

Jingbai Ma
2013-09-17T04:36:44Z

On 09/17/2013 03:45 AM, Dave Anderson wrote:
> Recent testing on very large memory systems dictates that an
> update is required for the compressed kdump header generated
> by makedumpfile.
>
> The dumpfile header has this field, which was inherited from
> the old "diskdump" facility:
>
> struct disk_dump_header {
> ...
> unsigned int max_mapnr; /* = max_mapnr */
> ...
>
> and which, among other things, is used by the crash utility as a
> delimiter to determine whether a physical address read request is
> legitimate. And obviously the field cannot handle PFN values greater
> than 32-bits.
>
> The makedumpfile source code does have its own max_mapnr representation
> in its DumpInfo structure in "makedumpfile.h":
>
> struct DumpInfo {
> ...
> unsigned long long max_mapnr; /* number of page descriptor */
> ...
>
> But in its "diskdump_mod.h" file, it carries forward the old diskdump
> header format, which has the 32-bit field:
>
> struct disk_dump_header {
> ...
> unsigned int max_mapnr; /* = max_mapnr */
> ...
>
> And here in "makedumpfile.c", the inadvertent truncation occurs
> when the PFN is greater than 32-bits:
>
> int
> write_kdump_header(void)
> {
> ...
> dh->max_mapnr = info->max_mapnr;
> ...
>
> The 32-bit field has also been carried forward into the SADUMP header
> as well, which has this in "sadump_mod.h":
>
> struct sadump_header {
> ...
> uint32_t max_mapnr; /* = max_mapnr */
> ...
>
> And when these header structures change, the crash utility will need
> to be changed accordingly.
>
> Preferably for backwards-compatibility, a new header_version can be
> created, with the new expanded field located in the kdump_sub_header
> so that the original base structure can remain as-is. But I leave that
> up to the maintainers.
>
> Thanks,
> Dave
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>

For the persistent data structures, we should use more precision
declaration int32_t, int64_t, uint64_t instead of ambiguous int, long
int, long long int.
For example, we can change structure disk_dump_header as below:
struct disk_dump_header {
char signature[SIG_LEN]; /* = "KDUMP " */
int32_t header_version; /* Dump header version */
struct new_utsname utsname; /* copy of
system_utsname */
struct timeval timestamp; /* Time stamp */
uint32_t status; /* Above flags */
int32_t block_size; /* Size of a block in
byte */
int32_t sub_hdr_size; /* Size of arch dependent
header in blocks */
uint32_t bitmap_blocks; /* Size of Memory bitmap in
block */
uint64_t max_mapnr; /* = max_mapnr */
uint32_t total_ram_blocks;/* Number of blocks
should be
written */
uint32_t device_blocks; /* Number of total
blocks in
* the dump device */
uint32_t written_blocks; /* Number of written
blocks */
uint32_t current_cpu; /* CPU# which handles
dump */
int32_t nr_cpus; /* Number of CPUs */
struct task_struct *tasks[0];
};

--
Thanks,
Jingbai Ma