Hello Philipp, Thanks for reviewing the patch. Thanks, Tao Liu On Mon, Sep 27, 2021 at 5:35 PM Philipp Rudo <prudo@xxxxxxxxxx> wrote: > > Hi Tao, > > On Thu, 23 Sep 2021 21:46:14 +0800 > Tao Liu <ltao@xxxxxxxxxx> wrote: > > > 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. > > 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@xxxxxxxxxx> > > 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 > > > > > > -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility