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

[Tao Liu]

Hi lijiang,

On Fri, Oct 1, 2021 at 10:12 AM lijiang <lijiang at redhat.com> wrote:
>
>
>
> 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.
>

The original symname_hash_search() doesn't handle the same-name
collision either, just return the first found one. I think there are
other functions which handle the same-name collision. For example,
when we do:

crash> sym user_destory
ffffffff8bcfca40 (T) user_destroy
/usr/src/debug/kernel-3.10.0/linux-3.10.0.x86_64/security/keys/user_defined.c:
182
ffffffff8bd180c0 (t) user_destroy
/usr/src/debug/kernel-3.10.0/linux-3.10.0.x86_64/security/selinux/ss/policydb.c:
723

The collision will be handled by symbol_name_count() and
symbol_search_next() in symbols.c:cmd_sym. For symbol_search() it only
gives the start syment and symbol_search_next() will iterate the
latter ones with the same name.

>From my understanding, symbol_search() doesn't have to handle the
same-name collision itself. When the symbol to be searched is certain
to be unique, then return it. If uncertain, functions such as
symbol_name_count() and symbol_search_next() can help on that.

Thanks,
Tao Liu

> 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
> > >
> >