[Makedumpfile PATCH 0/2] Fix refiltering when kaslr enabled

[Pratyush Anand]

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
>