[Crash-utility] [PATCH v4 2/4] Get the absolute value of SYMNAME_HASH_INDEX

[Tao Liu]

Hello Lianbo and Philipp,

On Mon, Oct 11, 2021 at 6:10 PM Philipp Rudo <prudo at redhat.com> wrote:
>
> Hi Lianbo,
>
> sorry for the late response.
>
> On Sun, 3 Oct 2021 10:25:26 +0800
> lijiang <lijiang at redhat.com> wrote:
>
> > On Sat, Oct 2, 2021 at 12:10 AM Philipp Rudo <prudo at redhat.com> wrote:
> > >
> > > Hi Tao,
> > > Hi Lianbo,
> > >
> > > sorry for the late response.
> > >
> > > On Wed, 29 Sep 2021 15:46:19 +0800
> > > lijiang <lijiang at redhat.com> wrote:
> > >
> > > > > > > Currently, the macro is used twice in the symbols.c. This change seems
> > > > > > > not complicated. Any thoughts?
> > > > > >
> > > > > > do I understand your suggestion correct, you propose to replace the
> > > > > >
> > > > > >         #define SYMNAME_HASH_INDEX(name) ...
> > > > > >
> > > > > > in defs.h by something like
> > > > > >
> > > > > >         static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) {
> > > > > >                 return (name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) % SYMNAME_HASH);
> > > > > >         }
> > > > > >
> > > > > > in symbols.c? If so, I think that should be fine.
> > > > > >
> > > > Yes, you are right, Philipp.
> > > >
> > > > >
> > > > > Please correct me if I'm wrong. I don't think the function can work.
> > > > > Let's say name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) ==
> > > > > -1, then we will have:
> > > > >
> > > > > static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) {
> > > > >         return (-1) % 512;
> > > > > }
> > > > >
> > > > > The returned value is a very large number, and will overflow the array.
> > >
> > > @Tao: I don't think that should be possible here. In the function I
> > > have defined name as _unsigned_ char*. So the calculation should never
> > > become negative. But to be honest I haven't tested it.
> > >
> > > > No, this is a modulo operation, and its result will never exceed '512' anyway.
> > > > (unsigned long long)(-1) % 512 = 511
> > >
> > > @Lianbo: I don't think that's true. When the modulo is used with an
> > > negative dividend the result becomes negative. When this then gets
> > > casted to an unsigend integer type the result will become very large.
> > > That's also what a quick test showed me
> > >
> > > printf("%lli\n", ((-1LL) % 512));                       --> -1
> > > printf("%llu\n", (unsigned long long) ((-1LL) % 512));  --> 18446744073709551615
> > >
> >
> > Thank you for the reply, Tao and Philipp. Let's go back to the problem
> > itself and continue our discussion.
> > Does the following change work for you?
> >
> > ---
> >  defs.h    |  2 --
> >  symbols.c | 18 +++++++++++++++---
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/defs.h b/defs.h
> > index cbd45e52f9da..d7b9bcc89878 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -2728,8 +2728,6 @@ struct downsized {
> >          (((vaddr) >> machdep->pageshift) % SYMVAL_HASH)
> >
> >  #define SYMNAME_HASH (512)
> > -#define SYMNAME_HASH_INDEX(name) \
> > - ((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH)
> >
> >  #define PATCH_KERNEL_SYMBOLS_START  ((char *)(1))
> >  #define PATCH_KERNEL_SYMBOLS_STOP   ((char *)(2))
> > diff --git a/symbols.c b/symbols.c
> > index 69dccdb09d5f..8a2230df7712 100644
> > --- a/symbols.c
> > +++ b/symbols.c
> > @@ -1127,6 +1127,18 @@ symname_hash_init(void)
> >   st->__per_cpu_end = sp->value;
> >  }
> >
> > +unsigned int symname_hash_index(unsigned char *name)
> > +{
> > +     unsigned int len, value;
> > +
> > +     len = strlen(name);
> > +     if (!len)
> > +         error(FATAL, "The length of the symbol name is zero!\n");
> > +
> > +     value = name[len-1] * name[len/2];
> > +
> > +     return (name[0] ^ value) % (unsigned int)SYMNAME_HASH;
> > +}

I have sent the v5 patch upstream, which took the code, but made some
slight modification:

1) the code will receive compiling warnings for functions which call
symname_hash_index(), parameters as spn->name is "char *" however
symname_hash_index() receives "unsigned char *".

2) Compiling warning also happens in strlen(), which expects "const char *".

3) In v5 patch I have removed the "unsigned int" cast for
SYMNAME_HASH.There is no difference when looking at the disassembly
result for symname_hash_index() with or without "unsigned int" cast.

We can continue our discussion if you have more thoughts about it.

Thanks,
Tao Liu

> >  /*
> >   *  Install a single static kernel symbol into the symname_hash.
> >   */
> > @@ -1134,9 +1146,9 @@ static void
> >  symname_hash_install(struct syment *spn)
> >  {
> >   struct syment *sp;
> > -        int index;
> > +        unsigned int index;
> >
> > -        index = SYMNAME_HASH_INDEX(spn->name);
> > +        index = symname_hash_index(spn->name);
> >   spn->cnt = 1;
> >
> >          if ((sp = st->symname_hash[index]) == NULL)
> > @@ -1165,7 +1177,7 @@ symname_hash_search(char *name)
> >  {
> >   struct syment *sp;
> >
> > -        sp = st->symname_hash[SYMNAME_HASH_INDEX(name)];
> > +        sp = st->symname_hash[symname_hash_index(name)];
> >
> >   while (sp) {
> >   if (STREQ(sp->name, name))
>
> the patch looks good to me. Personally I would only update the
> definition of SYMNAME_HASH to be unsigned rather than doing the cast in
> symname_hash_index. But that's only a personal preference.
>
> Thanks
> Philipp
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>