> Hi Lianbo,
>
>> -----Original Message-----
>> 在 2019年12月26日 11:38, lijiang 写道:
>>> Hi, Kazu and Mikhail,
>>>
>>>> Hi Mikhail,
>>>>
>>>>> -----Original Message-----
>>>>> Hi,
>>>>>
>>>>> On 12.12.2019 17:12, Kazuhito Hagio wrote:
>>>>>> Hi Mikhail,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> Hello Kazu,
>>>>>>>
>>>>>>> I think we can try to generalize the kaslr offset extraction.
>>>>>>> I won't speak for other architectures, but for s390 that
>>>>>>> get_kaslr_offset_arm64()
>>>>>>> should work fine. The only concern of mine is this TODO statement:
>>>>>>>
>>>>>>> if (_text <= vaddr && vaddr <= _end) {
>>>>>>> DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>>>> return info->kaslr_offset;
>>>>>>> } else {
>>>>>>> /*
>>>>>>> * TODO: we need to check if it is vmalloc/vmmemmap/module
>>>>>>> * address, we will have different offset
>>>>>>> */
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> Could you explain this one?
>>>>>>
>>>>>> Probably it was considered that the check would be needed to support
>>>>>> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
>>>>>> originally.
>>>>>>
>>>>>> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
>>>>>> the offset we need is the one for symbol addresses in vmlinux only.
>>>>>> As I said below, module symbol addresses are retrieved from vmcore.
>>>>>> Other addresses should not be passed to the function for now, as far
>>>>>> as I know.
>>>>>>
>>>>>> So I think the TODO comment is confusing, and it would be better to
>>>>>> remove it or change it to something like:
>>>>>> /*
>>>>>> * Returns 0 if vaddr does not need the offset to be added,
>>>>>> * e.g. for module address.
>>>>>> */
>>>>>>
>>>>>> But if s390 uses get_kaslr_offset() in its arch-specific code to
>>>>>> adjust addresses other than kernel text address, we might need to
>>>>>> modify it for s390, not generalize it.
>>>>>
>>>>> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
>>>>> code.
>>>>
>>>> OK, I pushed a patch that generalizes it to my test repository.
>>>> Could you enable s390 to use it and test?
>>>> https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general
>>>>
>>>
>>> I enabled it on s390 as below and tested, it worked.
>
> Thank you for testing.
>
It's my pleasure.
>>>
>>> @@ -1075,7 +1075,7 @@ int is_iomem_phys_addr_s390x(unsigned long addr);
>>> #define get_phys_base() stub_true()
>>> #define get_machdep_info() get_machdep_info_s390x()
>>> #define get_versiondep_info() stub_true()
>>> -#define get_kaslr_offset(X) stub_false()
>>> +#define get_kaslr_offset(X) get_kaslr_offset_general(X)
>>> #define vaddr_to_paddr(X) vaddr_to_paddr_s390x(X)
>>>
>>> But, there is still a problem that needs to be improved. In the
>>> find_kaslr_offsets(),
>>> the value of SYMBOL(_stext) is always 0(zero) and it is passed to the
>>> get_kaslr_offset().
>>> For the following code in the get_kaslr_offset_general(), it does not work
>>> as expected.
>>> ...
>>> if (_text <= vaddr && vaddr <= _end)
>>> return info->kaslr_offset;
>>> else
>>> return 0;
>
> I don't know why the SYMBOL(_stext) is passed to the get_kaslr_offset()
> there, but
> since the return value of get_kaslr_offset() is not used in the
> find_kaslr_offsets(),
> it's meaningless and not harmful. So it is not worth doing
> READ_SYMBOL(_stext) there
> for now.
>
Sounds good.
>>
>> In addition, the above code confused me, it will always return 0 on
>> s390(please refer to my logs).
>
> The aim of get_kaslr_offset() here is only setting info->kaslr_offset to the
> value
> from vmcoreinfo for the SYMBOL_INIT() macro.
> (get_kaslr_offset() returns the kaslr offset in the resolve_config_entry().)
>
Thanks for your explanation, Kazu.
> But yeah, the get_kaslr_offset(SYMBOL(_stext)) is confusing and not good.
> Passing 0 might be a bit better..?
>
Yes, looks good to me.
Lianbo
> Thanks,
> Kazu
>
>>
>> Thanks.
>>
>>> ...
>>> Here is my log:
>>> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000,
>>> _end:10ba000, vaddr:0
>>>
>>> After applied the following patch, got the expected result.
>>> int
>>> find_kaslr_offsets()
>>> {
>>> @@ -3973,6 +4042,11 @@ find_kaslr_offsets()
>>> * called this function between open_vmcoreinfo() and
>>> * close_vmcoreinfo()
>>> */
>>> + READ_SYMBOL("_stext", _stext);
>>> + if (SYMBOL(_stext) == NOT_FOUND_SYMBOL) {
>>> + ERRMSG("Can't get the symbol of _stext.\n");
>>> + goto out;
>>> + }
>>> get_kaslr_offset(SYMBOL(_stext));
>>>
>>> Here is my log:
>>> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000,
>>> _end:10ba000, vaddr:67fbc000
>>>
>>> Basically, before using the value of SYMBOL(_stext), need to ensure that
>>> the SYMBOL(_stext) is parsed
>>> correctly.
>>>
>>> Thanks.
>>>
>>>> Thanks,
>>>> Kazu
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Kazu
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Mikhail
>>>>>>>
>>>>>>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
>>>>>>>> Hi Mikhail,
>>>>>>>>
>>>>>>>> Sorry for late reply.
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however
>>>>>>>>> no
>>>>>>>>> support has been added yet. This patch adds the arch specific function
>>>>>>>>> get_kaslr_offset() for s390x.
>>>>>>>>> Since the values in vmcoreinfo are already relocated, the patch is
>>>>>>>>> mainly relevant for vmlinux processing (-x option).
>>>>>>>>
>>>>>>>> In the current implementation of makedumpfile, the
>>>>>>>> get_kaslr_offset(vaddr)
>>>>>>>> is supposed to return the KASLR offset only when the offset is needed
>>>>>>>> to
>>>>>>>> add to the vaddr. So generally symbols from kernel (vmlinux) need it,
>>>>>>>> but
>>>>>>>> symbols from modules are resolved dynamically and don't need the
>>>>>>>> offset.
>>>>>>> \>
>>>>>>>> This patch always returns the offset if any, as a result, I guess this
>>>>>>>> patch
>>>>>>>> will not work as expected with module symbols in filter config file.
>>>>>>>>
>>>>>>>> So... How about making get_kaslr_offset_arm64() general for other archs
>>>>>>>> (get_kaslr_offset_general() or something), then using it also for s390?
>>>>>>>> If OK, I can do that generalization.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Kazu
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Philipp Rudo <pr...@linux.ibm.com>
>>>>>>>>> Signed-off-by: Mikhail Zaslonko <zaslo...@linux.ibm.com>
>>>>>>>>> ---
>>>>>>>>> arch/s390x.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>> makedumpfile.h | 3 ++-
>>>>>>>>> 2 files changed, 34 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/s390x.c b/arch/s390x.c
>>>>>>>>> index bf9d58e..892df14 100644
>>>>>>>>> --- a/arch/s390x.c
>>>>>>>>> +++ b/arch/s390x.c
>>>>>>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>>>>>>>> return TRUE;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +unsigned long
>>>>>>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
>>>>>>>>> +{
>>>>>>>>> + unsigned int i;
>>>>>>>>> + char buf[BUFSIZE_FGETS], *endp;
>>>>>>>>> +
>>>>>>>>> + if (!info->file_vmcoreinfo)
>>>>>>>>> + return FALSE;
>>>>>>>>> +
>>>>>>>>> + if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>>>>>>>>> + ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>>>>>>>>> + info->name_vmcoreinfo, strerror(errno));
>>>>>>>>> + return FALSE;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>>>>>>>>> + i = strlen(buf);
>>>>>>>>> + if (!i)
>>>>>>>>> + break;
>>>>>>>>> + if (buf[i - 1] == '\n')
>>>>>>>>> + buf[i - 1] = '\0';
>>>>>>>>> + if (strncmp(buf, STR_KERNELOFFSET,
>>>>>>>>> + strlen(STR_KERNELOFFSET)) == 0) {
>>>>>>>>> + info->kaslr_offset =
>>>>>>>>> + strtoul(buf + strlen(STR_KERNELOFFSET),
>>>>>>>>> &endp, 16);
>>>>>>>>> + DEBUG_MSG("info->kaslr_offset: %lx\n",
>>>>>>>>> info->kaslr_offset);
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + return info->kaslr_offset;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> static int
>>>>>>>>> is_vmalloc_addr_s390x(unsigned long vaddr)
>>>>>>>>> {
>>>>>>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>>>>>>> index ac11e90..26f6247 100644
>>>>>>>>> --- a/makedumpfile.h
>>>>>>>>> +++ b/makedumpfile.h
>>>>>>>>> @@ -1071,11 +1071,12 @@ unsigned long long
>>>>>>>>> vaddr_to_paddr_ppc(unsigned long vaddr);
>>>>>>>>> int get_machdep_info_s390x(void);
>>>>>>>>> unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>>>>>>>> int is_iomem_phys_addr_s390x(unsigned long addr);
>>>>>>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>>>>>>>> #define find_vmemmap() stub_false()
>>>>>>>>> #define get_phys_base() stub_true()
>>>>>>>>> #define get_machdep_info() get_machdep_info_s390x()
>>>>>>>>> #define get_versiondep_info() stub_true()
>>>>>>>>> -#define get_kaslr_offset(X) stub_false()
>>>>>>>>> +#define get_kaslr_offset(X) get_kaslr_offset_s390x(X)
>>>>>>>>> #define vaddr_to_paddr(X) vaddr_to_paddr_s390x(X)
>>>>>>>>> #define paddr_to_vaddr(X) paddr_to_vaddr_general(X)
>>>>>>>>> #define is_phys_addr(X) is_iomem_phys_addr_s390x(X)
>>>>>>>>> --
>>>>>>>>> 2.17.1
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>>
>>>
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>
>>
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
