sahara: auto-assemble minidump ELF after ramdump download#243
sahara: auto-assemble minidump ELF after ramdump download#243mukeshojha-linux wants to merge 1 commit into
Conversation
When the kernel minidump backend is active, the Sahara debug table contains a KELF region — a complete ELF core header describing the physical address and output offset of every other kernel region. Detect KELF after download and stitch all kernel regions into a single minidump.elf, ready for crash or gdb, without any manual concatenation. Devices without minidump enabled are unaffected. On Windows, inline minimal ELF64 definitions since <elf.h> is absent in MinGW environments. Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
| #include "qdl.h" | ||
| #include "oscompat.h" | ||
|
|
||
| #ifndef _WIN32 |
There was a problem hiding this comment.
#if defined(__linux__)
...
#endif
and this will address MacOS build failure as well
There was a problem hiding this comment.
The purpose of the include is to avoid duplicating definitions in the source file, as we already duplicated them below for the other OSs we could simply skip the #if and #include and just run off the same definition (unless there's some convenient way to get elf.h on Windows and MacOS)
| } | ||
|
|
||
| fseek(in, 0, SEEK_END); | ||
| kelf_size = ftell(in); |
There was a problem hiding this comment.
please check ftell() result for -1L
| while (written < phdrs[j].p_filesz) { | ||
| char zero = 0; | ||
|
|
||
| fwrite(&zero, 1, 1, out); |
There was a problem hiding this comment.
writes one byte at a time, maybe we better use chunked zero buffer for this instead, e.g. :
static const char zeros[4096] = { 0 };
size_t n = phdrs[j].p_filesz;
while (n > 0) {
size_t chunk = n > sizeof(zeros) ? sizeof(zeros) : n;
if (fwrite(zeros, 1, chunk, out) != chunk)
return -1;
n -= chunk;
}
or just use sparse hole, e.g. fseek() past the gap and write one sentinel byte
I'd also move this to a distinct function
| } | ||
|
|
||
| /* 2. Write vmcoreinfo immediately after KELF to complete the PT_NOTE */ | ||
| if (vmcore_idx >= 0) { |
There was a problem hiding this comment.
I'd move this logic for fopen() + chunked read/write + fclose() to a separate function (copy_file_to(out, path)), so we can avoid similar functionality duplication on lines 620-635
| } | ||
| fclose(in); | ||
|
|
||
| ehdr = kelf_buf; |
There was a problem hiding this comment.
please add sanity checks (in case ELF header is malformed) for ehdr:
ehdr->e_phoff > kelf_size || (size_t)ehdr->e_phnum * sizeof(Elf64_Phdr) > kelf_size - ehdr->e_phoff
| * All segments are written sequentially because meminspect assigns p_offset | ||
| * values in a strictly increasing, contiguous order. | ||
| */ | ||
| static void sahara_build_minidump_elf(const char *ramdump_path, const char *filter, |
There was a problem hiding this comment.
I first asked that you'd move this whole thing to a minidump.c that you call once sahara_run() returns. But I missed the fact that you're passing the sahara_debug_region64 table.
I still think it would be nice to split this out, as it does not relate to the sahara protocol directly. But I think it's reasonable to just call it from sahara.c directly, at least not very useful to try to get the table out to the caller at this time.
| if ((ssize_t)i == kelf_idx || strncmp(table[i].region, "md_K", 4) != 0) | ||
| continue; | ||
|
|
||
| if (sahara_debug64_filter(table[i].filename, filter)) |
There was a problem hiding this comment.
I guess this would make sense if I grab a minidump into a directory and then again grab another minidump with stricter filter?
Please add a comment about ensuring that stale files aren't picked up.
3 participants
When the kernel minidump backend is active, the Sahara debug table contains a KELF region — a complete ELF core header describing the physical address and output offset of every other kernel region.
Detect KELF after download and stitch all kernel regions into a single minidump.elf, ready for crash or gdb, without any manual concatenation. Devices without minidump enabled are unaffected.
On Windows, inline minimal ELF64 definitions since <elf.h> is absent in MinGW environments.
based on suggestion given here for meminspect patches
https://lore.kernel.org/lkml/abtlUQqMOxj5PwGB@baldur/