On Tue, Mar 29, 2022 at 2:38 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com> wrote:
-----Original Message-----
> >  Lianbo Jiang <lijiang@redhat.com> writes:
> >
> >  > The commit <cd8954023bd4> broke crash-utility on s390x and got the
> >  > following error:
> >  >
> >  >   crash: cannot resolve ".rodata"
> >  >
> >  > The reason is that all symbols containing a "." may be filtered out
> >  > on s390x. To prevent the current failure, a simple way is to check
> >  > whether the symbol ".rodata" exists before calculating the value of
> >  > a symbol.
> >  >
> >  > Fixes: cd8954023bd4 ("kernel: fix start-up time degradation caused by strings command")
> >  > Reported-by: Alexander Egorenkov <egorenar@linux.ibm.com>
> >  > Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> >  > ---
> >  >  kernel.c | 3 +++
> >  >  1 file changed, 3 insertions(+)
> >  >
> >  > diff --git a/kernel.c b/kernel.c
> >  > index 92434a3ffe2d..b504564846c7 100644
> >  > --- a/kernel.c
> >  > +++ b/kernel.c
> >  > @@ -11790,6 +11790,9 @@ int get_linux_banner_from_vmlinux(char *buf, size_t size)
> >  >       struct bfd_section *sect;
> >  >       long offset;
> >  >
> >  > +     if (!symbol_exists(".rodata"))
> >  > +             return FALSE;
> >  > +
> >  >       sect = bfd_get_section_by_name(st->bfd, ".rodata");
> >  >       if (!sect)
> >  >               return FALSE;
> >  > --
> >  > 2.20.1
> >
> >  thanks! This works on s390x.
> >
> > Sorry, my reply was truncated. How about the following changes?
> >
> >
> >  diff --git a/kernel.c b/kernel.c
> >  index 92434a3ffe2d..b504564846c7 100644
> >  --- a/kernel.c
> >  +++ b/kernel.c
> >  @@ -11790,6 +11790,9 @@ int get_linux_banner_from_vmlinux(char *buf, size_t size)
> >    struct bfd_section *sect;
> >    long offset;
> >
> >  + if (!symbol_exists(".rodata"))

kernel_symbol_exists() is better.

Good understanding, Kazu. Looks good to me.
 
> >  + return FALSE;
> >  +
> >    sect = bfd_get_section_by_name(st->bfd, ".rodata");
> >    if (!sect)
> >    return FALSE;
> >  diff --git a/s390.c b/s390.c
> >  index 078b1a25724e..42f5cc63ae52 100644
> >  --- a/s390.c
> >  +++ b/s390.c
> >  @@ -442,6 +442,9 @@ s390_verify_symbol(const char *name, ulong value, char type)
> >    if (strstr(name, "L2\002") == name)
> >        return FALSE;
> >
> >  + if (STREQ(name, ".rodata"))
> >  + return TRUE;
> >  +
> >    /* throw away all symbols containing a '.' */
> >    for(i = 0; i < strlen(name);i++){
> >    if(name[i] == '.')
> >  diff --git a/s390x.c b/s390x.c
> >  index c07d283d7f52..d7ee3755fc0b 100644
> >  --- a/s390x.c
> >  +++ b/s390x.c
> >  @@ -1087,6 +1087,9 @@ s390x_verify_symbol(const char *name, ulong value, char type)
> >    if (strstr(name, "L2\002") == name)
> >        return FALSE;
> >
> >  + if (STREQ(name, ".rodata"))
> >  + return TRUE;
> >  +
> >    /* throw away all symbols containing a '.' */
> >    for(i = 0; i < strlen(name);i++){
> >    if(name[i] == '.')
>
> Looks good to me.
> Tested on s390x, works with get_linux_banner_from_vmlinux().

I also could not find the reason why it throws away those symbols in the changelog [1]
and the list archive [2].  So I think it might be safe to not change it.. but
as Alex is ok with this,

Acked-by: Kazuhito Hagio <k-hagio-ab@nec.com>

Thanks for the response, I will apply this later.
 
 
[1] https://crash-utility.github.io/crash.changelog.html
[2] e.g. https://www.google.com/search?q=site%3Ahttps%3A%2F%2Flistman.redhat.com%2F+%22s390x_verify_symbol%22

Thanks!
Kazu