Re: [PATCH v4 4/4] Add check if an syment element is installed one more time

[lijiang <lijiang@xxxxxxxxxx>]

On Mon, Sep 27, 2021 at 10:00 PM Tao Liu <ltao@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >
> > On Mon, 27 Sep 2021 12:00:26 +0800
> > lijiang <lijiang@xxxxxxxxxx> wrote:
> >
> > > > Date: Sat, 18 Sep 2021 15:59:32 +0800
> > > > From: Tao Liu <ltao@xxxxxxxxxx>
> > > > To: crash-utility@xxxxxxxxxx
> > > > Subject: [PATCH v4 4/4] Add check if an syment element
> > > >         is      installed one more time
> > > > Message-ID: <20210918075932.132339-5-ltao@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxx
> > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > >
> >
> > --
> > Crash-utility mailing list
> > Crash-utility@xxxxxxxxxx
> > https://listman.redhat.com/mailman/listinfo/crash-utility
> >
>

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility