[GIT PULL] string fixes for v6.15-rc1

[GIT PULL] string fixes for v6.15-rc1

[Kees Cook]

Hi Linus,

Please pull this pair of string API fixes for v6.15-rc1.

Thanks!

-Kees

The following changes since commit b688f369ae0d5d25865f5441fa62e54c7d5d0de6:

  compiler_types: Introduce __nonstring_array (2025-03-12 13:21:09 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/hardening-v6.15-rc1-fix1

for you to fetch changes up to 48ac25ef250da7478ea54afbb1aced40c342e78d:

  kasan: Add strscpy() test to trigger tag fault on arm64 (2025-04-03 10:19:26 -0700)

----------------------------------------------------------------
string fixes for v6.15-rc1

- Add wcslen() to support more Clang libcalls (Nathan Chancellor)

- Fix KASAN MTE exceptions during strscpy() (Peter Collingbourne,
  Vincenzo Frascino)

----------------------------------------------------------------
Nathan Chancellor (2):
      include: Move typedefs in nls.h to their own header
      lib/string.c: Add wcslen()

Peter Collingbourne (1):
      string: Add load_unaligned_zeropad() code path to sized_strscpy()

Vincenzo Frascino (1):
      kasan: Add strscpy() test to trigger tag fault on arm64

 include/linux/nls.h       | 19 +------------------
 include/linux/nls_types.h | 26 ++++++++++++++++++++++++++
 include/linux/string.h    |  2 ++
 lib/string.c              | 24 +++++++++++++++++++++---
 mm/kasan/kasan_test_c.c   | 20 ++++++++++++++++++++
 5 files changed, 70 insertions(+), 21 deletions(-)
 create mode 100644 include/linux/nls_types.h

-- 
Kees Cook

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

On Sun, 6 Apr 2025 at 10:54, Kees Cook <kees@kernel.org> wrote:
>
> - Add wcslen() to support more Clang libcalls (Nathan Chancellor)

Oh Christ.

Does clang not know how expensive function calls can be?

I really think the right fix here would have been to say "don't do
that", rather than make that function available.

When the function implementation is the stupid version, there is *no*
advantage to a function call.

In user mode, if you have

 (a) long strings

 (b) you use some optimized vectorized string library

 (c) you don't have the problems with function calls being potentially
expensive due to return prediction CPU workarounds

this compiler optimization may make sense.

But in the kernel, it *never* makes sense, because none of those three
issues are ever true.

So this change is just *stupid*.

And I'm not pulling stupid code. The one-liner rto just disable an
optimization that isn't an optimization is the right thing to do.

And if LTO has problems with that, then LTO needs to be fixed, dammit.
It's stupid without LTO, it's doubly stupid with extra
"optimizations".

               Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

On Sun, 6 Apr 2025 at 12:04, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> When the function implementation is the stupid version, there is *no*
> advantage to a function call.

Side note: the reason we don't do -freestanding is that some functions
we really *really* want the compiler to optimize away the function
entirely.

Things like regular 'strlen()' are commonly used on compile-time fixed
strings. It needs to be dealt with by the compiler.

Same goes for the basic memcpy and memset functions.

So what the kernel really wants is basically "-freestanding" but with
a very targeted set of functions still dealt with by the compiler -
not ever because they should be turned into function calls, but
because they can be simplified to some entirely different form at
compile time.

(There are a _couple_ of other cases, but not many: we end up having
things like "constant prefix matching" patterns, where we want the
compiler to not only notice that 'strlen()' on a constant string is
just a compile-time constant value, but then also want to simplify
'strncmp()' or 'memcmp()' with a small constant size to basically be a
much simpler thing)

But we do not want the compiler to create out-of-line function calls
for code that wasn't a function call to begin with.  That's pretty
much the exact *opposite* of what we actually want.

We did try "-freestanding" at some point, because it's the obvious way
to avoid the compiler messing this very basic thing up, but those
"just deal with simple cases directly" are really really important.

I wish there was a version of -freestanding that did that "only
simplify calls" version, not the "recognize patterns and turn them
into calls that didn't exist".

            Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Kees Cook]

On April 6, 2025 12:04:04 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sun, 6 Apr 2025 at 10:54, Kees Cook <kees@kernel.org> wrote:
>>
>> - Add wcslen() to support more Clang libcalls (Nathan Chancellor)
>
>Oh Christ.
>
>Does clang not know how expensive function calls can be?
>
>I really think the right fix here would have been to say "don't do
>that", rather than make that function available.

I should have said "libcall optimizations". It's not just blindly constructing calls. This is the same kind of thing that has been heavily discussed before for bcmp() and stpcpy(). The resolution from those threads was to add the symbol, as the least of many bad options:
https://lore.kernel.org/all/CAK7LNAQHj-GeqH_5WpKo7gA6qZAiX8OOxxnL1v-SNZwRHFSXQQ@mail.gmail.com/


-- 
Kees Cook

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

On Sun, 6 Apr 2025 at 18:33, Kees Cook <kees@kernel.org> wrote:
>
> I should have said "libcall optimizations". It's not just blindly constructing calls.

But it's *WRONG*.

It's stupid. It's not an optimization, it makes things worse.

> This is the same kind of thing that has been heavily discussed before for bcmp() and stpcpy()

And it makes a bit more sense at least for stpcpy(), because the
implementation there is basically "strlen+memcpy". Both of which we
want the compiler to work on - even if we're not interested in it ever
using stpcpy().

IOF, for stpcpy, there's at least a *reason* for the compiler to do it.

For something like wcslen() the answer is "DON'T DO THIS". Because
there is absolutely zero upside to trying to recognize this pattern,
and there is real downside.

See?

Don't work around the compiler doing stupid things. Fix the compiler
options to tell the compiler to "Don'tDoThatThen(tm)".

            Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

Hi Linus,

On Sun, Apr 06, 2025 at 07:04:29PM -0700, Linus Torvalds wrote:
> On Sun, 6 Apr 2025 at 18:33, Kees Cook <kees@kernel.org> wrote:
> >
> > I should have said "libcall optimizations". It's not just blindly constructing calls.
> 
> But it's *WRONG*.
> 
> It's stupid. It's not an optimization, it makes things worse.
> 
> > This is the same kind of thing that has been heavily discussed before for bcmp() and stpcpy()
> 
> And it makes a bit more sense at least for stpcpy(), because the
> implementation there is basically "strlen+memcpy". Both of which we
> want the compiler to work on - even if we're not interested in it ever
> using stpcpy().
> 
> IOF, for stpcpy, there's at least a *reason* for the compiler to do it.
> 
> For something like wcslen() the answer is "DON'T DO THIS". Because
> there is absolutely zero upside to trying to recognize this pattern,
> and there is real downside.
> 
> See?
> 
> Don't work around the compiler doing stupid things. Fix the compiler
> options to tell the compiler to "Don'tDoThatThen(tm)".

So I do not necessarily disagree with you in the general sense for these
types of optimizations but I figured that in this case, where this
optimization only gets applied twice in a single translation unit
throughout the entire kernel from what I can tell, the overhead was
unlikely to matter much and it felt less problematic to just add the
function. If this is still genuinely unacceptable in your eyes in spite
of that, so be it.

I will admit I did not actually test if '-fno-builtin-wcslen' would not
work with LTO when I wrote the commit message (I merely drew on the
experience for bcmp() several years ago). Now that I have, it appears
to, at least for the simple arm64 allmodconfig case that I tested.
Looking into it, it looks like '-fno-builtin-*' started being honored
properly for LTO with [1] in LLVM 10 and fixed/adjusted for inlining in
[2] in LLVM 11.

So would the following change be acceptable? I can draft up a commit
message and send it along today if so.

diff --git a/Makefile b/Makefile
index 38689a0c3605..a137de124897 100644
--- a/Makefile
+++ b/Makefile
@@ -1057,6 +1057,10 @@ KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)
 KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow)
 KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow)
 
+# Ensure clang does not transform certain loops into calls to wcslen() after
+# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
+KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen
+
 # disable invalid "can't wrap" optimizations for signed / pointers
 KBUILD_CFLAGS	+= -fno-strict-overflow
 
---

[1]: https://github.com/llvm/llvm-project/commit/878ab6df033d44430939c02075ee00800995dc3b
[2]: https://github.com/llvm/llvm-project/commit/f9ca75f19bab639988ebbe68c81d07babd952afb

Cheers,
Nathan

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

On Mon, 7 Apr 2025 at 10:37, Nathan Chancellor <nathan@kernel.org> wrote:
>
> So would the following change be acceptable? I can draft up a commit
> message and send it along today if so.

Absoluterly. That's the right thing to do.

> +# Ensure clang does not transform certain loops into calls to wcslen() after
> +# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
> +KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen

I think you could just use

     KBUILD_CFLAGS += $(call cc-option, -fno-builtin-wcslen)

instead, and not use some version check?

          Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

On Mon, Apr 07, 2025 at 12:02:04PM -0700, Linus Torvalds wrote:
> On Mon, 7 Apr 2025 at 10:37, Nathan Chancellor <nathan@kernel.org> wrote:
> > +# Ensure clang does not transform certain loops into calls to wcslen() after
> > +# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
> > +KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen
> 
> I think you could just use
> 
>      KBUILD_CFLAGS += $(call cc-option, -fno-builtin-wcslen)
> 
> instead, and not use some version check?

Sure, I just figured this has not been a problem up until this point so
applying it for older versions of clang should not be necessary (since
the optimization that introduces it does not exist) but also not be
harmful. However, I would rather not call out to the compiler since we
know it is supported with all versions of clang (and GCC but it
obviously does not need it) so maybe just

    KBUILD_CFLAGS-$(CONFIG_CC_IS_CLANG) += -fno-builtin-wcslen

or

    ifdef CONFIG_CC_IS_CLANG
    KBUILD_CFLAGS += -fno-builtin-wcslen
    endif

or if you do want this for GCC too, unconditionally adding it should be
fine too.

    KBUILD_CFLAGS += -fno-builtin-wcslen

No strong opinion, let me know your preference and I will make it so.

Cheers,
Nathan

Re: [GIT PULL] string fixes for v6.15-rc1

[David Laight]

On Sun, 6 Apr 2025 19:04:29 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

...
> For something like wcslen() the answer is "DON'T DO THIS". Because
> there is absolutely zero upside to trying to recognize this pattern,
> and there is real downside.

gcc also has a nasty habit of converting:
	for (i = 0; i < len; i++)
		dst[i] = src[i];
into a call to memcpy().
If I wanted a memcpy() call I'd write one - so will most people.
But if 'len' is very small (may even known to be less than, say, 4)
you really want the loop - which is why it was written.

I've even seen (not gcc) it converted to a 'rep movsw' 'rep movsb'
pair at a time when a P4 might have been a likely target cpu.
The 0 to 3 byte 'rep movsb' had a setup cost of IIRC 150 clocks.

	David

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

 On Mon, 7 Apr 2025 at 12:25, Nathan Chancellor <nathan@kernel.org> wrote:
>
> or if you do want this for GCC too, unconditionally adding it should be
> fine too.

I think if unconditionally works, that's probably the best option
simply because it's the simplest option.

But I don't see 'wcslen' in the gcc docs, which was why I was assuming
it wanted that "check if it works" thing with "$(call cc-option,...)"

I don't think we need to call out the particular compiler, since the
argument against using it is not compiler-specific per se.

             Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

On Mon, Apr 07, 2025 at 01:25:04PM -0700, Linus Torvalds wrote:
> I think if unconditionally works, that's probably the best option
> simply because it's the simplest option.
> 
> But I don't see 'wcslen' in the gcc docs, which was why I was assuming
> it wanted that "check if it works" thing with "$(call cc-option,...)"

It appears that neither gcc nor clang warn for "invalid" libcall values
to '-fno-builtin-*':

$ echo 'int main(void) { return 0; }' | clang -fno-builtin-ireallydonotexist -o /dev/null -S -x c -
$ echo 'int main(void) { return 0; }' | gcc -fno-builtin-ireallydonotexist -o /dev/null -S -x c -

> I don't think we need to call out the particular compiler, since the
> argument against using it is not compiler-specific per se.

Sounds good, I will ultimately make this:

  # Ensure compilers do not transform certain loops into calls to wcslen()
  KBUILD_CFLAGS += -fno-builtin-wcslen

Cheers,
Nathan