Re: [PATCH v4] libfs: Attempt exact-match comparison first during casefolded lookup

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, 24 Jan 2024 at 10:13, Gabriel Krisman Bertazi <krisman@suse.de> wrote:
>
> Just for completeness, below the version I intend to apply to
> unicode/for-next , which is the v2, plus the comments you and Eric
> requested. That is, unless something else comes up.

Looks ok to me.

My one comment is actually unrelated to the new code, just because the
patch touches this code too:

>         if (len <= DNAME_INLINE_LEN - 1) {
>                 memcpy(strbuf, str, len);
>                 strbuf[len] = 0;
> -               qstr.name = strbuf;
> +               str = strbuf;
>                 /* prevent compiler from optimizing out the temporary buffer */
>                 barrier();

The reason for this whole mess is that allegedly utf8_strncasecmp() is
not safe if the buffer changes under it.

At least that's what the comment says.

But honestly, I don't see it.

I think the whole "copy to a stable buffer" code can and should just
be removed as voodoo programming.

*If* the buffer is actually changing, the name lookup code will just
retry, so whether the return value is correct or not is irrelevant.

All that matters is that the code honors the str/len constraint, and
not blow up - even if the data inside that str/len buffer might not be
stable.

I don't see how the utf8 code could possibly mess up.

That code goes back to commit

  2ce3ee931a09 ("ext4: avoid utf8_strncasecmp() with unstable name")
  fc3bb095ab02 ("f2fs: avoid utf8_strncasecmp() with unstable name")

and I think it's bogus.

Eric - the string *data* may be unsafe, but the string length passed
to the utf8 routines is not changing any more (since it was loaded
long ago).

And honestly, no amount of "the data may change" should possibly ever
cause the utf8 code to then ignore the length that was passed in.

                Linus