On Wednesday 24 May 2017 11:41 AM, Atsushi Kumagai wrote: >> On Tuesday 23 May 2017 08:39 AM, Atsushi Kumagai wrote: >>>>>>>>> Thanks for your report, I have received this. >>>>>>>>> I'm on vacation until Mar 8, I'll review it when I return from vacation. >>>>>>>> >>>>>>>> Any further comment on it? >>>>>>>> Otherwise, I will send a v2 after accommodating concern from Xunlei. >>>>>>> >>>>>>> Unfortunately, it doesn't seem like I can make time anymore for review this week, >>>>>>> but at least this patch doesn't seem to work in my environment (linux 4.8 without kaslr). >>>>>>> Do you have any ideas ? >>>>>> >>>>>> I see, why it would have caused. I have not tested this case, but I hope my v2 >>>>>> should not have this issue. >>>>> >>>>> Umm, v2 still doesn't work in my environment... >>>>> It seems that I have to investigate this deeper. >>>> >>>> Hummm, I thought we would see file_vmcoreinfo as NULL in >>>> get_kaslr_offset_x86_64() in your case. However, it's not true. >>>> >>>> I think, it will have to be initialized with NULL in main(). >>>> >>>> Can you please try following fixup on top of this series: >>> >>> I found the cause, please see below: >>> >>> initial() >>> + find_kaslr_offsets() >>> + open_vmcoreinfo() >>> + get_kaslr_offset() // set info->kaslr_offset >>> + close_vmcoreinfo() >>> gather_filter_info() >>> (snip) >>> + resolve_config_entry() >>> + get_kaslr_offset() // occur SIGSEGV since info->file_vmcoreinfo is closed >> >> Since info->file_vmcoreinfo is closed,therefore it should be NULL. But >> currently, close_vmcoreinfo() does not set it to NULL. We should also set >> info->file_vmcoreinfo to NULL in close_vmcoreinfo() apart from main(). > > Sure, uninitialized info->file_vmcoreinfo is a general issue, > I'll fix it in another patch. I will be sending v2 today which should also fix issue seen by Hatayama, Daisuke. http://lists.infradead.org/pipermail/kexec/2017-May/018833.html ~Pratyush > >>> >>> >>> The cause code is in [PATCH v2 1/2], >>> >>> diff --git a/erase_info.c b/erase_info.c >>> index f2ba914..60abfa1 100644 >>> --- a/erase_info.c >>> +++ b/erase_info.c >>> @@ -1088,6 +1088,7 @@ resolve_config_entry(struct config_entry *ce, unsigned long long base_vaddr, >>> ce->line, ce->name); >>> return FALSE; >>> } >>> + ce->sym_addr += get_kaslr_offset(ce->sym_addr); >>> ce->type_name = get_symbol_type_name(ce->name, >>> DWARF_INFO_GET_SYMBOL_TYPE, >>> &ce->size, &ce->type_flag); >>> >>> >>> I think we should use info->kaslr_offset instead of get_kaslr_offset(), >>> how about you ? >> >> Actually, we are not sure at this point that ce->sym_addr is a kernel linear >> address. It could be a module address and that can have different >> randomization offset. > > I see, I misunderstood that the randomization offset is same in any case. > >> My intention is to calculate all types of kaslr offsets in >> find_kaslr_offsets() very early and then store them in different "info" >> elements. Now, whenever we call get_kaslr_offset() we would return right >> offset as per the input address. >> I have very little idea about x86. So, I have left a TODO to calculate offset >> for non-linear addresses. >> >>> >>> BTW, I'm not sure why you didn't meet this issue... >> >> Because, I tested with kaslr kernel, so when get_kaslr_offset(ce->sym_addr) >> was called, I already had a valid info->kaslr_offset. >> >> So, what about following fixup > > As mentioned at the beginning, the fix below is not your fault, > so I'll merge your patches as it is into v1.6.2. > >> diff --git a/makedumpfile.c b/makedumpfile.c >> index 57235690569e..4986d098d69a 100644 >> --- a/makedumpfile.c >> +++ b/makedumpfile.c >> @@ -8685,6 +8685,7 @@ close_vmcoreinfo(void) >> if(fclose(info->file_vmcoreinfo) < 0) >> ERRMSG("Can't close the vmcoreinfo file(%s). %s\n", >> info->name_vmcoreinfo, strerror(errno)); >> + info->file_vmcoreinfo = NULL; >> } >> >> void >> @@ -11076,6 +11077,7 @@ main(int argc, char *argv[]) >> strerror(errno)); >> goto out; >> } >> + info->file_vmcoreinfo = NULL; >> info->fd_vmlinux = -1; >> info->fd_xen_syms = -1; >> info->fd_memory = -1; > > However, I think [PATCH 2/2] should be dropped since the alternative patch > has appeared, is it ok with you ? > > https://github.com/pratyushanand/makedumpfile/commit/16655ce1f4c2da8d4066072db2a03c84bf2553fe > > Thanks, > Atsushi Kumagai > >> >> ~Pratyush >> >> _______________________________________________ >> kexec mailing list >> kexec at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/kexec > > > > _______________________________________________ > kexec mailing list > kexec at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec >
