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

[Tao Liu]

Hello Philipp,

Thanks for reviewing the patch.

Thanks,
Tao Liu

On Mon, Sep 27, 2021 at 5:35 PM Philipp Rudo <prudo at redhat.com> wrote:
>
> Hi Tao,
>
> On Thu, 23 Sep 2021 21:46:14 +0800
> Tao Liu <ltao at redhat.com> wrote:
>
> > Hi Philipp,
> >
> > Thanks  for reviewing the patch!
> >
> > On Thu, Sep 23, 2021 at 8:19 PM Philipp Rudo <prudo at redhat.com> wrote:
> > >
> > > Hi Tao,
> > >
> > > On Sat, 18 Sep 2021 15:59:32 +0800
> > > Tao Liu <ltao at redhat.com> wrote:
> > >
> > > > 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;
> > > >
> > > >          index = SYMNAME_HASH_INDEX(spn->name);
> > >
> > > hmm... not sure if this is a little bit over the top. The idea I had
> > > was in your v3 simply replace
> > >
> > > assert(sp != spn);
> > >
> > > by
> > >
> > > if (sp == spn) {
> > >         error(WARNING, "Symbol %s already installed in symname_hash\n",
> > >               sp->name);
> > >         continue;
> > > }
> > >
> >
> > It may not be easy to replace with "check if sp == spn and continue".
> > For example, if we already have
> > 3 syments installed, which all have the same name, such as
> > (sp1{cnt == 3, name == "str"}, sp2{cnt == 3, name == "str"}, sp3{cnt
> > == 3, name == "str"})
> > in the hashtable, and we will install sp3 again(sp3 == spn) into the hashtable.
> >
> > The cnt will get increased for sp1 and sp2 in the following code:
> > while (sp) {
> >         if (sp == spn) {
> >              error(....);
> >              continue;
> >         }
> >         if (STREQ(sp->name, spn->name)) {
> >                 sp->cnt++;
> >                 spn->cnt++;
> >         }
> > .....
> > }
> >
> > However, when iteration reaches sp3, it will not increase cnt and not
> > install spn. Thus we will have
> > (sp1{cnt == 4}, sp2{cnt == 4}, sp3{cnt == 3}) in the hashtable, cnt
> > gets corrupted. In other words,
> > we need to revert all the previous cnt++ when we reach sp == spn. It
> > will require
> > more code to implement the 'revert' operation, and it is not as clear
> > as syment_is_installed
> > check.
>
> you are right. I totally missed that. In fact my suggestion is even
> worse. At the beginning the function sets
>
> spn->cnt = 1;
>
> So in the scenario you described above spn3->cnt == 1 should be true...
>
> Having that said, your patch looks good to me
>
> Reviewed-by: Philipp Rudo <prudo at redhat.com>
>
> Thanks
> Philipp
>
> > > That's less code plus the warning makes it easier to detect that there
> > > is a problem (for me the case sp == spn is a sign for a bug in crash).
> > > What do you think?
> > >
> >
> > From my point,
> > 1) assert() check: Use least code, simple and clear, but too strict.
> > 2) syment_is_installed check: Use more code, clear, acceptable to me.
> > 3) introduce 'cnt++ revert' operation: I haven't think of a better way, from
> >     the current inspection, it uses more code, and is not elegant.
> > What do you think?
> >
> > Thanks,
> > Tao Liu
> >
> > > Thanks
> > > Philipp
> > >
> >
>