Generally I agree with it. I spotted it too when worked on my change. It just needs 8 times more RAM then what we have now to overflow, so I preferred my patch to be minimal. Just going that way, why not make uh_hashmask also unsigned? Signed bit mask makes even less sense. You've changed hval from int in one place, but not other(s).

In D19148#409622, @mav wrote:

Generally I agree with it. I spotted it too when worked on my change. It just needs 8 times more RAM then what we have now to overflow, so I preferred my patch to be minimal. Just going that way, why not make uh_hashmask also unsigned?

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is stil a valid value (?).

Signed bit mask makes even less sense. You've changed hval from int in one place, but not other(s).

OK, I caught the missing change in hash_expand(), thanks.

In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is stil a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

In D19148#409628, @mav wrote:

In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is still a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

A zero value would come from an error or a malicious user.

I am not involved with the theory here (heck ... I am a mechanical engineer not a CS guy), could we use +1 instead of -1 for the hash mask? If the idea is just to have a unique value, +1 won't overflow and having that much memory would cause other troubles anyway.

In D19148#409749, @pfg wrote:

In D19148#409628, @mav wrote:

In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is still a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

A zero value would come from an error or a malicious user.

I don't see how that can happen. This functionality is entirely internal to the kernel.

I am not involved with the theory here (heck ... I am a mechanical engineer not a CS guy), could we use +1 instead of -1 for the hash mask? If the idea is just to have a unique value, +1 won't overflow and having that much memory would cause other troubles anyway.

No, that would break the algorithm. The idea is to create a mask by subtracting 1 from a power of 2. I agree with mav that there's no reason to have a signed mask.