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

[Philipp Rudo]

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
>