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

[lijiang]

On Mon, Sep 27, 2021 at 10:00 PM Tao Liu <ltao at redhat.com> wrote:
>
> 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.
>
Thank you for the explanation, Tao.

> 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.
>
If that's true, it may have a hash collision because here is a simple
string hash, the
symname_hash_search() always returns the first symbol entry with the
'name',  seems
that it doesn't consider the hash collision with the same 'name' when
searching for the
symbol entry with the name, right?

Currently, the symname_hash_search() handles the hash collision with the
different name
when searching for the symbol entry.

Thanks.
Lianbo

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20211001/524651b7/attachment.htm>