We need a room for the new flag (introduced later) in pc->flags. Signed-off-by: Oleg Nesterov <oleg(a)redhat.com&gt; --- defs.h | 2 +- filesys.c | 2 +- main.c | 2 +- remote.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/defs.h b/defs.h index 56ae06c..cc5db51 100644 --- a/defs.h +++ b/defs.h @@ -212,7 +212,6 @@ struct number_option { #define DEVMEM (0x2000000ULL) #define REM_LIVE_SYSTEM (0x4000000ULL) #define NAMELIST_LOCAL (0x8000000ULL) -#define MEMSRC_LOCAL (0x10000000ULL) #define NAMELIST_SAVED (0x20000000ULL) #define DUMPFILE_SAVED (0x40000000ULL) #define UNLINK_NAMELIST (0x80000000ULL) @@ -523,6 +522,7 @@ struct program_context { #define SNAP (0x20000ULL) #define EXCLUDED_VMEMMAP (0x40000ULL) #define is_excluded_vmemmap() (pc->flags2 & EXCLUDED_VMEMMAP) +#define MEMSRC_LOCAL (0x80000ULL) char *cleanup; char *namelist_orig; char *namelist_debug_orig; diff --git a/filesys.c b/filesys.c index 9b59998..f05a8cd 100644 --- a/filesys.c +++ b/filesys.c @@ -162,7 +162,7 @@ fd_init(void) static void memory_source_init(void) { - if (REMOTE() && !(pc->flags & MEMSRC_LOCAL)) + if (REMOTE() && !(pc->flags2 & MEMSRC_LOCAL)) return; if (pc->flags & KERNEL_DEBUG_QUERY) diff --git a/main.c b/main.c index 821bb4e..a4db88d 100644 --- a/main.c +++ b/main.c @@ -1294,7 +1294,7 @@ dump_program_context(void) if (pc->flags & REM_LIVE_SYSTEM) sprintf(&buf[strlen(buf)], "%sREM_LIVE_SYSTEM", others++ ? "|" : ""); - if (pc->flags & MEMSRC_LOCAL) + if (pc->flags2 & MEMSRC_LOCAL) sprintf(&buf[strlen(buf)], "%sMEMSRC_LOCAL", others++ ? "|" : ""); if (pc->flags & NAMELIST_LOCAL) diff --git a/remote.c b/remote.c index 2938669..88a326b 100644 --- a/remote.c +++ b/remote.c @@ -2537,7 +2537,7 @@ remote_fd_init(void) error(INFO, "too many dumpfile/memory arguments\n"); program_usage(SHORT_FORM); } - pc->flags |= MEMSRC_LOCAL; + pc->flags2 |= MEMSRC_LOCAL; if (pc->flags & (DEVMEM|MEMMOD)) { if (!get_proc_version()) error(INFO, "/proc/version: %s\n", @@ -3025,7 +3025,7 @@ dumpfile_save: return FALSE; } - if (pc->flags & MEMSRC_LOCAL) { + if (pc->flags2 & MEMSRC_LOCAL) { error(INFO, "%s is a local file\n", pc->dumpfile); return FALSE; } -- 2.5.0

Change fd_init() and memory_source_init() to check LOCAL_ACTIVE(). We need this for the last patch in series. The 1st change is obviously safe, fd_init() sets LIVE_SYSTEM | MEMSRC_LOCAL and nobody else can set LIVE_SYSTEM if we get here. The 2nd one should be correct even if REMOTE() is true, we check MEMSRC_LOCAL at the start of memory_source_init(). --- filesys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/filesys.c b/filesys.c index 9ea8f7e..3b2ce05 100644 --- a/filesys.c +++ b/filesys.c @@ -135,7 +135,7 @@ fd_init(void) pc->nfd = -1; } - if (ACTIVE() && !(pc->namelist_debug || pc->system_map)) { + if (LOCAL_ACTIVE() && !(pc->namelist_debug || pc->system_map)) { memory_source_init(); match_proc_version(); } @@ -169,7 +169,7 @@ memory_source_init(void) if (pc->flags & KERNEL_DEBUG_QUERY) return; - if (ACTIVE()) { + if (LOCAL_ACTIVE()) { if (pc->mfd != -1) /* already been here */ return; -- 2.5.0

Use pid == 1 and /sbin/init as tt->this_task instead, pc->program_pid can't be used. We could probably leave tt->this_task = NO_TASK, but we need to initialize CURRENT_CONTEXT() anyway and we need something for refresh_context(), to me "init" looks better than (say) FIRST_CONTEXT() == swapper/0. However, this means that /init/sbin will be always reported as "active", see the next patch. Signed-off-by: Oleg Nesterov <oleg(a)redhat.com&gt; --- task.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/task.c b/task.c index 7b01951..fd4b700 100644 --- a/task.c +++ b/task.c @@ -124,7 +124,6 @@ task_init(void) long tss_offset, thread_offset; long eip_offset, esp_offset, ksp_offset; struct gnu_request req; - ulong active_pid; if (!(tt->idle_threads = (ulong *)calloc(NR_CPUS, sizeof(ulong)))) error(FATAL, "cannot malloc idle_threads array"); @@ -493,7 +492,9 @@ task_init(void) tt->flags &= ~(TASK_REFRESH|TASK_REFRESH_OFF); if (ACTIVE()) { - active_pid = REMOTE() ? pc->server_pid : pc->program_pid; + ulong active_pid = REMOTE() ? pc->server_pid : + LOCAL_ACTIVE() ? pc->program_pid : + 1; set_context(NO_TASK, active_pid); tt->this_task = pid_to_task(active_pid); } -- 2.5.0

Obviously it can only work if LOCAL_ACTIVE() == T. Signed-off-by: Oleg Nesterov <oleg(a)redhat.com&gt; --- kernel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel.c b/kernel.c index 5a392fc..d53249b 100644 --- a/kernel.c +++ b/kernel.c @@ -8452,7 +8452,7 @@ panic_this_kernel(void) { pid_t zero_pid = 0; - if (DUMPFILE()) + if (!LOCAL_ACTIVE()) error(FATAL, "cannot panic a dumpfile!\n"); if (!(pc->flags & MFD_RDWR) || (pc->flags & MEMMOD)) -- 2.5.0

Preparation. This way "ramdump" array is ready for use right after is_ramdump(), and this also simplifies the code a little bit. Signed-off-by: Oleg Nesterov <oleg(a)redhat.com&gt; --- ramdump.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/ramdump.c b/ramdump.c index ab93767..c6a8a85 100644 --- a/ramdump.c +++ b/ramdump.c @@ -59,29 +59,19 @@ static void alloc_elf_header(Elf64_Ehdr *ehdr, ushort e_machine) ehdr->e_shstrndx = 0; } -static int alloc_program_headers(Elf64_Phdr *phdr) +static void alloc_program_headers(Elf64_Phdr *phdr) { unsigned int i; - struct stat64 st; for (i = 0; i < nodes; i++) { phdr[i].p_type = PT_LOAD; - - if (0 > stat64(ramdump[i].path, &st)) { - error(INFO, "ramdump stat failed\n"); - return -1; - } - - phdr[i].p_filesz = st.st_size; + phdr[i].p_filesz = ramdump[i].end_paddr + 1 - ramdump[i].start_paddr; phdr[i].p_memsz = phdr[i].p_filesz; phdr[i].p_vaddr = 0; phdr[i].p_paddr = ramdump[i].start_paddr; - ramdump[i].end_paddr = ramdump[i].start_paddr + st.st_size - 1; phdr[i].p_flags = PF_R | PF_W | PF_X; phdr[i].p_align = 0; } - - return 0; } static char *write_elf(Elf64_Phdr *load, Elf64_Ehdr *e_head, size_t data_offset) @@ -219,8 +209,7 @@ char *ramdump_to_elf(void) load = (Elf64_Phdr *)ptr; - if (alloc_program_headers(load)) - goto end; + alloc_program_headers(load); offset += sizeof(Elf64_Phdr) * nodes; ptr += sizeof(Elf64_Phdr) * nodes; @@ -248,6 +237,7 @@ int is_ramdump(char *p) char *x = NULL, *y = NULL, *pat; size_t len; char *pattern; + struct stat64 st; int err = 0; if (nodes || !strchr(p, '@')) @@ -277,6 +267,10 @@ int is_ramdump(char *p) "ramdump %s open failed:%s\n", ramdump[nodes - 1].path, strerror(errno)); + if (fstat64(ramdump[nodes - 1].rfd, &st) < 0) + error(FATAL, "ramdump stat failed\n"); + ramdump[nodes - 1].end_paddr = + ramdump[nodes - 1].start_paddr + st.st_size - 1; } pat = NULL; -- 2.5.0

----- Original Message ----- Hi Oleg, This looks pretty good, but I do have a couple of questions/comments. In [PATCH v2 02/10]: @@ -124,6 +124,7 @@ fd_init(void) if (!pc->dumpfile) { pc->flags |= LIVE_SYSTEM; + pc->flags2 |= MEMSRC_LOCAL; get_live_memory_source(); } Now that MEMSRC_LOCAL has been effectively moved out of the way, why is it being set above in fd_init()? And in [PATCH v2 10/10]: @@ -428,6 +428,17 @@ main(int argc, char **argv) "too many dumpfile arguments\n"); program_usage(SHORT_FORM); } + + if (ACTIVE()) { + pc->flags |= LIVEDUMP; + /* disable get_live_memory_source() logic in fd_init() */ + pc->dumpfile = "livedump"; + pc->readmem = read_ramdump; + pc->writemem = NULL; + optind++; + continue; + } The pc->dumpfile should point to "/tmp/MEM" or however it was invoked on the command line, i.e., just like any other dumpfile, regardless of whether it is live or not. And if pc->dumpfile were changed to be the actual filename, then the following qualifier shouldn't be necessary: @@ -209,7 +209,8 @@ memory_source_init(void) } if (pc->dumpfile) { - if (!file_exists(pc->dumpfile, NULL)) + if (!(pc->flags & LIVEDUMP) && + !file_exists(pc->dumpfile, NULL)) error(FATAL, "%s: %s\n", pc->dumpfile, strerror(ENOENT)); And this change in [PATCH vs 9/10]: @@ -219,8 +209,7 @@ char *ramdump_to_elf(void) load = (Elf64_Phdr *)ptr; - if (alloc_program_headers(load)) - goto end; + alloc_program_headers(load); offset += sizeof(Elf64_Phdr) * nodes; ptr += sizeof(Elf64_Phdr) * nodes; causes this compiler warning if "make warn" is used: $ make warn ... [ cut ] ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 ramdump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security ramdump.c: In function 'ramdump_to_elf': ramdump.c:230:1: warning: label ‘end’ defined but not used [-Wunused-label] end: ^ ... We'll also need to vet every instance of ACTIVE() to make sure there are no other dependencies on the local system. For example, this one comes to mind, where x86_64_calc_phys_base() looks at /proc/iomem: if (ACTIVE()) { if ((iomem = fopen("/proc/iomem", "r")) == NULL) return; ... And if you bring up a cscope session, and search for the "/proc/" strings, there may be other local /proc filesystem references that aren't obviously inside of "if (ACTIVE())". I did check some of them, and most would only be relevant/called if it's a local active instance, but some may not. But anyway, this looks good for starters. Talk to you next week, Dave

Hi Dave, Following the mails quiet late, and it looks like lot of discussion has gone in between you and Oleg.right now ramdump.c just addresses ARM since we work on ARM arch. if we have to support x86/_64, headers need to be dealt differently. when I had written code (before ramdump was submitted), I just wrote it with  ARM in mind,  but I think, there is a design change/tiny framework where arch specific implementation could hook their functions such as alloc_elf_headers. let me know what you think.  Regards,Oza. On Saturday, 30 April 2016, 20:39, paawan oza <paawan1982(a)yahoo.com&gt; wrote: Hi Oleg, Just was curious, what kind of addition your patch is bringing ? Broadcom brought the support for raw ramdump, well you know justpreparing ELF header and sparse support. so just wanted to understand how what you patch does, it addresses live dump ? could you elaborate on it ? Regards,Oza. On Saturday, 30 April 2016, 1:27, Dave Anderson <anderson(a)redhat.com&gt; wrote: ----- Original Message ----- Hi Oleg, This looks pretty good, but I do have a couple of questions/comments. In [PATCH v2 02/10]: @@ -124,6 +124,7 @@ fd_init(void)                 if (!pc->dumpfile) {                         pc->flags |= LIVE_SYSTEM; +                      pc->flags2 |= MEMSRC_LOCAL;                         get_live_memory_source();                 } Now that MEMSRC_LOCAL has been effectively moved out of the way, why is it being set above in fd_init()? And in [PATCH v2 10/10]: @@ -428,6 +428,17 @@ main(int argc, char **argv)                                         "too many dumpfile arguments\n");                                         program_usage(SHORT_FORM);                         } + +                      if (ACTIVE()) { +                              pc->flags |= LIVEDUMP; +                              /* disable get_live_memory_source() logic in fd_init() */ +                              pc->dumpfile = "livedump"; +                              pc->readmem = read_ramdump; +                              pc->writemem = NULL; +                              optind++; +                              continue; +                      } The pc->dumpfile should point to "/tmp/MEM" or however it was invoked on the command line, i.e., just like any other dumpfile, regardless of whether it is live or not. And if pc->dumpfile were changed to be the actual filename, then the following qualifier shouldn't be necessary: @@ -209,7 +209,8 @@ memory_source_init(void)         }         if (pc->dumpfile) { -              if (!file_exists(pc->dumpfile, NULL)) +              if (!(pc->flags & LIVEDUMP) && +                  !file_exists(pc->dumpfile, NULL))                         error(FATAL, "%s: %s\n", pc->dumpfile,                                 strerror(ENOENT)); And this change in [PATCH vs 9/10]: @@ -219,8 +209,7 @@ char *ramdump_to_elf(void)         load = (Elf64_Phdr *)ptr; -      if (alloc_program_headers(load)) -              goto end; +      alloc_program_headers(load);         offset += sizeof(Elf64_Phdr) * nodes;         ptr += sizeof(Elf64_Phdr) * nodes; causes this compiler warning if "make warn" is used: $ make warn ... [ cut ] ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6  ramdump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security ramdump.c: In function 'ramdump_to_elf': ramdump.c:230:1: warning: label ‘end’ defined but not used [-Wunused-label] end: ^ ... We'll also need to vet every instance of ACTIVE() to make sure there are no other dependencies on the local system.  For example, this one comes to mind, where x86_64_calc_phys_base() looks at /proc/iomem:         if (ACTIVE()) {                 if ((iomem = fopen("/proc/iomem", "r")) == NULL)                     return;         ... And if you bring up a cscope session, and search for the "/proc/" strings, there may be other local /proc filesystem references that aren't obviously inside of "if (ACTIVE())".  I did check some of them, and most would only be relevant/called if it's a local active instance, but some may not.  But anyway, this looks good for starters. Talk to you next week,   Dave -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

----- Original Message ----- Hi Oza, Note that Rabin Vincent added support for 32-bit MIPS in crash-7.1.1. And given that the ELF format is 64-bit LSB, the only difference would be the ehdr->e_machine field. So I believe that setting e_machine to EM_X86_64 in ramdump_to_elf() should just work. If it doesn't work, the vmcore will be rejected during initialization. Dave

----- Original Message ----- MEMSRC_LOCAL has always only been set in remote_fd_init() to signal that, for whatever reason, the memory source is local but the vmlinux is remote. But remoted_fd_init() would only be called if the deprecated remote crash daemon was actually running on another machine and a command line argument specified it. So for all practical purposes MEMSRC_LOCAL is an unused no-op, and should not be set above. Dave

----- Original Message ----- I have no problem with LOCAL_ACTIVE() being defined like that. However, now I'm wondering whether "LIVEDUMP" should be renamed to differentiate it from LIVE_DUMP. LIVE_DUMP means one of two things: (1) On an s390x, a dumpfile was take by firmware of a running system. (2) The snap.so extension module was used to create an ELF vmcore of a running system. So both are static dumpfiles, but were taken while the system was running live. In this case, it's more like /dev/mem et al, and it's non-static. Maybe "LIVE_RAMDUMP"? Dave