> 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 <prudo@xxxxxxxxxxxxx> >>>>>>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@xxxxxxxxxxxxx> >>>>>>>>> --- >>>>>>>>> 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@xxxxxxxxxxxxxxxxxxx >>>> http://lists.infradead.org/mailman/listinfo/kexec >>>> >>> >>> >>> _______________________________________________ >>> kexec mailing list >>> kexec@xxxxxxxxxxxxxxxxxxx >>> http://lists.infradead.org/mailman/listinfo/kexec >>> >> > _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec