Hi Philipp,
Thanks for reviewing the patch!
On Thu, Sep 23, 2021 at 8:19 PM Philipp Rudo <prudo@xxxxxxxxxx> wrote:
>
> Hi Tao,
>
> On Sat, 18 Sep 2021 15:59:32 +0800
> Tao Liu <ltao@xxxxxxxxxx> 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@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;
> >
> > 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.
> 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
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
