Re: [Crash-utility] [PATCH v4 2/4] Get the absolute value of SYMNAME_HASH_INDEX
On Sat, Oct 2, 2021 at 12:10 AM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
Hi Lianbo,
sorry for the late response.
On Wed, 29 Sep 2021 15:46:19 +0800
lijiang <lijiang(a)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;
+}
/*
* 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))
--
2.17.1
Thanks.
Lianbo
> --
> Crash-utility mailing list
> Crash-utility(a)redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>