Re: broken hash use in fs/cifs/dfs_cache.c

Al Viro
2021-03-22T20:24:54Z

On Mon, Mar 22, 2021 at 07:38:15PM +0100, Aurélien Aptel wrote:
> Al Viro <viro@zeniv.linux.org.uk> writes:
> > Either the key comparison or the hash function is wrong here. *IF* something
> > external guarantees the full match, we don't need strcasecmp() - strcmp()
> > would work. Otherwise, the hash function needs to be changed.
>
> I think here we need to make the hash case-insensitive.
>
> Perhaps calling jhash() with lower-cased bytes like so (pseudo-code):

[snip]

Then you really do not want to recalculate it again and again.
Look:

__lookup_cache_entry() calculates it for the key

lookup_cache_entry() contains
if (cnt < 3) {
h = cache_entry_hash(path, strlen(path));
ce = __lookup_cache_entry(path);
goto out;
}

... and
ce = __lookup_cache_entry(npath);
if (!IS_ERR(ce)) {
h = cache_entry_hash(npath, strlen(npath));
break;
}
(in a loop, at that)

Take a look at that the aforementioned loop:
h = cache_entry_hash(npath, strlen(npath));
e = npath + strlen(npath) - 1;
while (e > s) {
char tmp;

/* skip separators */
while (e > s && *e == sep)
e--;
if (e == s)
goto out;

tmp = *(e+1);
*(e+1) = 0;

ce = __lookup_cache_entry(npath);
if (!IS_ERR(ce)) {
h = cache_entry_hash(npath, strlen(npath));
break;
}

*(e+1) = tmp;
/* backward until separator */
while (e > s && *e != sep)
e--;
}
We call __lookup_cache_entry() for shorter and shorter prefixes of
npath. They get NUL-terminated for the duration of __lookup_cache_entry(),
then reverted to the original. What for? cache_entry_hash() already
gets length as explicit argument. And strcasecmp() is trivially
replaced with strncasecmp().

Just have __lookup_cache_entry() take key, hash and length. Then it
turns into
len = e + 1 - s;
hash = cache_entry_hash(path, len);
ce = __lookup_cache_entry(path, hash, len);
if (!IS_ERR(ce)) {
h = hash;
break;
}
and we are done. No need to modify npath contents, undo the modifications
or *have* npath in the first place - the reason the current variant needs to
copy path is precisely that it goes to those contortions.

Incidentally, you also have a problem with trailing separators - anything
with those inserted into hash won't be found by lookup_cache_entry(),
since you trim the trailing separators from the key on searches.