[Crash-utility] [PATCH v4 4/4] Add check if an syment element is installed one more time

[Tao Liu]

Hello Lianbo and Philipp,

syment_is_installed is used to check if the specific syment has been
installed before. It checks whether the syment address and the given
address are the same.
symname_hash_search is used to search the syment which has the given
name. It checks whether the name field of the syment and the given
name are the same.

Please note the hash table can have more than 1 syments which have the
same name but differ in address. What We want to avoid is the case if
a syment has been installed before, it should never be installed
again. So here we care about syment address, instead of syment name.

Like Philipp said, if we use symname_hash_search to implement
syment_is_installed, we will use extra code to check the address of
symnet returned by symname_hash_search. The code will not be as clear
as we directly use syment_is_installed.

Thanks,
Tao Liu

On Mon, Sep 27, 2021 at 5:54 PM Philipp Rudo <prudo at redhat.com> wrote:
>
> On Mon, 27 Sep 2021 12:00:26 +0800
> lijiang <lijiang at redhat.com> wrote:
>
> > > Date: Sat, 18 Sep 2021 15:59:32 +0800
> > > From: Tao Liu <ltao at redhat.com>
> > > To: crash-utility at redhat.com
> > > Subject: [Crash-utility] [PATCH v4 4/4] Add check if an syment element
> > >         is      installed one more time
> > > Message-ID: <20210918075932.132339-5-ltao at redhat.com>
> > > Content-Type: text/plain; charset="US-ASCII"
> > >
> > > symname_hash_install won't check if spn has been installed before. If
> > > it does, the second install will corrupt the hash table as well as
> > > spn->cnt counting. This patch adds the check to avoid such risks.
> > >
> > > Signed-off-by: Tao Liu <ltao at redhat.com>
> > > ---
> > >  symbols.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/symbols.c b/symbols.c
> > > index f7157b1..6d12c55 100644
> > > --- a/symbols.c
> > > +++ b/symbols.c
> > > @@ -1147,6 +1147,20 @@ mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
> > >                 symname_hash_remove(st->mod_symname_hash, sp);
> > >  }
> > >
> > > +static inline int
> > > +syment_is_installed(struct syment *table[], struct syment *spn)
> > > +{
> > > +       struct syment *sp;
> > > +       int index;
> > > +
> > > +       index = SYMNAME_HASH_INDEX(spn->name);
> > > +       for (sp = table[index]; sp; sp = sp->name_hash_next) {
> > > +               if (sp == spn)
> > > +                       return TRUE;
> > > +       }
> > > +       return FALSE;
> > > +}
> > > +
> > >  /*
> > >   *  Install a single static kernel symbol into the symname_hash.
> > >   */
> > > @@ -1156,7 +1170,7 @@ symname_hash_install(struct syment *table[], struct syment *spn)
> > >         struct syment *sp;
> > >          int index;
> > >
> > > -       if (!spn)
> > > +       if (!spn || syment_is_installed(table, spn))
> > >                 return;
> >
> > Here, why don't we call the existing symname_hash_search() and
> > redefine a new syment_is_installed()? Could you please describe more
> > details?
>
> I thought about that, too. In my opinion the problem with reusing
> symname_hash_search is that it searches for symbols with the same name.
> But the name doesn't necessarily have to be unique. So when
> symname_hash_search returns a symbol you still need to check if it is
> the same symbol and if not iterate through all symbols with the same
> name to check if any of them is. In the end I think that code will be
> more complex than having this additional syment_is_installed.
>
> Thanks
> Philipp
>
> >
> > Thanks.
> > Lianbo
> >
> > >
> > >          index = SYMNAME_HASH_INDEX(spn->name);
> > > --
> > > 2.29.2
> >
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://listman.redhat.com/mailman/listinfo/crash-utility
> >
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>