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.
> >
> > @@ -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.
>
> 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().)
But yeah, the get_kaslr_offset(SYMBOL(_stext)) is confusing and not good.
Passing 0 might be a bit better..?
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
