rust: kasan/kbuild: fix missing flags on first build
Subsystems (3)
Miguel Ojeda
maintainerPATCH 1/1 rust: kasan/kbuild: fix missing flags on first build
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepare
Then the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN rustc flags are added in the second run:
-Cllvm-args=-asan-instrumentation-with-call-threshold=10000
-Cllvm-args=-asan-stack=0
-Cllvm-args=-asan-globals=1
-Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated target.json file. That file is not ready by the time rustc gets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:
$ ... --target=./scripts/target.json ... -Cllvm-args=...
error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that: rustc is aware of the -Cllvm-args regardless of the --target (e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.
Thus filter out the target.
This does mean that rustc-option cannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.
We could only filter in the case target.json is used, to make it work in more cases, but then it would be harder to notice that it may not work in a couple architectures.
---
By the way, I noticed that we are not getting asan-instrument-allocas enabled in neither C nor Rust -- upstream LLVM renamed it in commit 8176ee9b5dda ("[asan] Rename asan-instrument-allocas -> asan-instrument-dynamic-allocas")). But it happened a very long time ago (9 years ago), and the addition in the kernel is fairly old too, in 342061ee4ef3 ("kasan: support alloca() poisoning"). I assume it should either be renamed or removed? Happy to send a patch if so.
scripts/Makefile.compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/Makefile.compiler +1 -1
@@ -80,7 +80,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3)) # TODO: remove RUSTC_BOOTSTRAP=1 when we raise the minimum GNU Make version to 4.4 __rustc-option = $(call try-run,\ echo '#![allow(missing_docs)]#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1\ - $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null,$(2)) $(3)\ + $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null --target=%,$(2)) $(3)\ --crate-type=rlib --out-dir=$(TMPOUT) --emit=obj=- - >/dev/null,$(3),$(4)) # rustc-option
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 --
2.49.0
Quoted 72 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.We could only filter in the case
target.jsonis used, to make it work in more cases, but then it would be harder to notice that it may not work in a couple architectures.Cc Matthew Maurer <[email protected]>Cc Sami Tolvanen <[email protected]>Fixes e3117404b411 ("kbuild: rust: Enable KASAN support")Signed-off-by Miguel Ojeda <[email protected]>---
By the way, I noticed that we are not gettingasan-instrument-allocasenabled in neither C nor Rust -- upstream LLVM renamed it in commit8176ee9b5dda("[asan] Rename asan-instrument-allocas -> asan-instrument-dynamic-allocas")). But it happened a very long time ago (9 years ago), and the addition in the kernel is fairly old too, in342061ee4ef3("kasan: supportalloca()poisoning"). I assume it should either be renamed or removed? Happy to send a patch if so.scripts/Makefile.compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)scripts/Makefile.compiler +0
-0
scripts/Makefile.compiler ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3)) · line 84
Quoted 6 lines
scripts/Makefile.compiler +1
-1@@ -80,7 +80,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3)) # TODO: remove RUSTC_BOOTSTRAP=1 when we raise the minimum GNU Make version to 4.4 __rustc-option = $(call try-run,\ echo '#![allow(missing_docs)]#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1\ - $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null,$(2)) $(3)\ + $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null --target=%,$(2)) $(3)\ --crate-type=rlib --out-dir=$(TMPOUT) --emit=obj=- - >/dev/null,$(3),$(4))
The problem with this change is that some rustc flags will only be valid on some platforms. For example, if we check if a -Zsanitizer=shadow-call-stack is available, it will fail for non aarch64 targets. I don't think we're currently directly detecting any of these, because all of the stuff we're using is known present by virtue of minimum compiler version, which means we can possibly get away with this change for now. That said, this isn't a long term solution unless we are getting rid of target.json files altogether, as one of the main adaptations we've been putting in those is to enable additional target features.
Quoted 6 lines
scripts/Makefile.compiler +0
-0# rustc-optionbase-commit:
0af2f6be1b4281385b618cb86ad946eded089ac8--
2.49.0
Quoted 3 lines · Matthew Maurer
The problem with this change is that some
rustcflags will only be valid on some platforms. For example, if we check if a
Indeed -- this limitation is acknowledged in the commit message. The priority is fixing the issue, not future features/needs, so I went with this.
In any case, if we need to test a flag that requires the target, it is likely we can do the check based on the Rust version, the architecture, the Rust's LLVM version, etc.
As for target.jsons, the plan is indeed to get rid of them, since they are permanently unstable. So any features used by the target.jsons need to be requested to upstream Rust (e.g. flags) so that we can do the same without a custom target specification, so that eventually we can remove all of them (and thus rustc-option will work in all cases again).
Thanks for taking a look!
Cheers,
Miguel
Miguel Ojeda
AppliedQuoted 2 lines · Miguel Ojeda
Thus filter out the target.
Applied to rust-fixes -- thanks everyone!
I am applying this one fairly quickly, to start to get wider testing, but it would be nice to get some tags, so I am happy to rebase to add them for the next day or two.
Cheers,
Miguel
Quoted 51 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.We could only filter in the case
target.jsonis used, to make it work in more cases, but then it would be harder to notice that it may not work in a couple architectures.
Quoted 5 lines
Cc Matthew Maurer <[email protected]>Cc Sami Tolvanen <[email protected]>Fixes e3117404b411 ("kbuild: rust: Enable KASAN support")Signed-off-by Miguel Ojeda <[email protected]>
I've boot-tested Android's KASAN configuration with this patch, and it continues to work. It also passes Android CI [1].
Alice
Yo,
Quoted 40 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.
Quoted 8 lines
Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.
Hmm, while this might be true, I think it should not actually have been true. Commit ca627e636551e ("rust: cfi: add support for CFI_CLANG with Rust") added a cc-option check to the rust kconfig symbol, checking if the c compiler supports the integer normalisations stuff:
depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
and also sets the relevant options in the makefile:
ifdef CONFIG_RUST
# Always pass -Zsanitizer-cfi-normalize-integers as CONFIG_RUST selects # CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
RUSTC_FLAGS_CFI := -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
KBUILD_RUSTFLAGS += $(RUSTC_FLAGS_CFI)
export RUSTC_FLAGS_CFI
endif
but it should also have added a rustc-option check as, unfortunately, support for kcfi in rustc is target specific. This results in build breakages where the arch supports CFI_CLANG and RUST, but the target in use does not have the kcfi flag set.
I attempted to fix this by adding:
diff --git a/arch/Kconfig b/arch/Kconfig index d1b4ffd6e0856..235709fb75152 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -916,6 +916,7 @@ config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG + depends on $(rustc-option,-C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers) depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \
but of course this does not work for cross compilation, as you're
stripping the target information out and so the check passes on my host
even though my intended
RUSTC_BOOTSTRAP=1 rustc -C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers -Ctarget-cpu=generic-rv64 --target=riscv64imac-unknown-none-elf is a failure.
I dunno too much about rustc itself, but I suspect that adding kcfi to the target there is a "free" win, but that'll take time to trickle down and the minimum version rustc version for the kernel isn't going to have that.
I'm not really sure what your target.json suggestion below is, so just reporting so that someone that understands the alternative solutions can fix this.
Cheers,
Conor.
Quoted 38 lines
We could only filter in the case
target.jsonis used, to make it work in more cases, but then it would be harder to notice that it may not work in a couple architectures.Cc Matthew Maurer <[email protected]>Cc Sami Tolvanen <[email protected]>Fixes e3117404b411 ("kbuild: rust: Enable KASAN support")Signed-off-by Miguel Ojeda <[email protected]>---
By the way, I noticed that we are not gettingasan-instrument-allocasenabled in neither C nor Rust -- upstream LLVM renamed it in commit8176ee9b5dda("[asan] Rename asan-instrument-allocas -> asan-instrument-dynamic-allocas")). But it happened a very long time ago (9 years ago), and the addition in the kernel is fairly old too, in342061ee4ef3("kasan: supportalloca()poisoning"). I assume it should either be renamed or removed? Happy to send a patch if so.scripts/Makefile.compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)scripts/Makefile.compiler +1
-1@@ -80,7 +80,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3)) # TODO: remove RUSTC_BOOTSTRAP=1 when we raise the minimum GNU Make version to 4.4 __rustc-option = $(call try-run,\ echo '#![allow(missing_docs)]#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1\ - $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null,$(2)) $(3)\ + $(1) --sysroot=/dev/null $(filter-out --sysroot=/dev/null --target=%,$(2)) $(3)\ --crate-type=rlib --out-dir=$(TMPOUT) --emit=obj=- - >/dev/null,$(3),$(4)) # rustc-optionbase-commit:
0af2f6be1b4281385b618cb86ad946eded089ac8--
2.49.0
Alice Ryhl
reviewerQuoted 88 lines · Conor Dooley
Yo,
Quoted 40 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.Quoted 8 lines
Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.Hmm, while this might be true, I think it should not actually have been true. Commit
ca627e636551e("rust: cfi: add support forCFI_CLANGwith Rust") added a cc-option check to the rust kconfig symbol, checking if the c compiler supports the integer normalisations stuff:depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)and also sets the relevant options in the makefile:
ifdefCONFIG_RUST
# Always pass -Zsanitizer-cfi-normalize-integers asCONFIG_RUSTselects #CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
RUSTC_FLAGS_CFI:= -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
KBUILD_RUSTFLAGS+= $(RUSTC_FLAGS_CFI)
exportRUSTC_FLAGS_CFI
endif
but it should also have added a rustc-option check as, unfortunately, support for kcfi in rustc is target specific. This results in build breakages where the arch supportsCFI_CLANGand RUST, but the target in use does not have the kcfi flag set.
I attempted to fix this by adding:diff --git a/arch/Kconfig b/arch/Kconfig index d1b4ffd6e0856..235709fb75152 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -916,6 +916,7 @@ config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG + depends on $(rustc-option,-C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers) depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \but of course this does not work for cross compilation, as you're
stripping the target information out and so the check passes on my host
even though my intended
RUSTC_BOOTSTRAP=1 rustc -C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers -Ctarget-cpu=generic-rv64 --target=riscv64imac-unknown-none-elf is a failure.
Quoted 8 lines
I dunno too much about rustc itself, but I suspect that adding kcfi to the target there is a "free" win, but that'll take time to trickle down and the minimum version rustc version for the kernel isn't going to have that.
I'm not really sure what your target.json suggestion below is, so just reporting so that someone that understands the alternative solutions can fix this.
Probably right now we have to do this cfg by
depends on CONFIG_ARM
to prevent riscv if rustc has the missing setting set on riscv. Once we add it to riscv, we change it to
depends on CONFIG_ARM || (RUSTC_VERSION >= ??? || CONFIG_RISCV)
Alice
Quoted 94 lines · Alice Ryhl
Quoted 93 lines · Conor Dooley
Yo,
Quoted 40 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.Quoted 8 lines
Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.Hmm, while this might be true, I think it should not actually have been true. Commit
ca627e636551e("rust: cfi: add support forCFI_CLANGwith Rust") added a cc-option check to the rust kconfig symbol, checking if the c compiler supports the integer normalisations stuff:depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)and also sets the relevant options in the makefile:
ifdefCONFIG_RUST
# Always pass -Zsanitizer-cfi-normalize-integers asCONFIG_RUSTselects #CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
RUSTC_FLAGS_CFI:= -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
KBUILD_RUSTFLAGS+= $(RUSTC_FLAGS_CFI)
exportRUSTC_FLAGS_CFI
endif
but it should also have added a rustc-option check as, unfortunately, support for kcfi in rustc is target specific. This results in build breakages where the arch supportsCFI_CLANGand RUST, but the target in use does not have the kcfi flag set.
I attempted to fix this by adding:diff --git a/arch/Kconfig b/arch/Kconfig index d1b4ffd6e0856..235709fb75152 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -916,6 +916,7 @@ config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG + depends on $(rustc-option,-C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers) depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \but of course this does not work for cross compilation, as you're
stripping the target information out and so the check passes on my host
even though my intended
RUSTC_BOOTSTRAP=1 rustc -C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers -Ctarget-cpu=generic-rv64 --target=riscv64imac-unknown-none-elf is a failure.I dunno too much about rustc itself, but I suspect that adding kcfi to the target there is a "free" win, but that'll take time to trickle down and the minimum version rustc version for the kernel isn't going to have that.
Quoted 7 lines
Quoted 3 lines
I'm not really sure what your target.json suggestion below is, so just reporting so that someone that understands the alternative solutions can fix this.
Probably right now we have to do this cfg by
depends on
CONFIG_ARM
It's valid on x86 too, right?
Quoted 5 lines
to prevent riscv if rustc has the missing setting set on riscv. Once we add it to riscv, we change it to
depends on CONFIG_ARM || (RUSTC_VERSION >= ??? || CONFIG_RISCV)
I kinda shied away from something like this since there was already a cc-option on the other half and checking different versions per arch becomes a mess - but yeah it kinda is a no-brainer to do it here when rustc-option is kinda broken.
I guess the temporary fix is then:
config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC
def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG depends on ARM64 || x86_64 depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \ (!GCOV_KERNEL && !KASAN_GENERIC && !KASAN_SW_TAGS)
because there's no 32-bit target with SanitizerSet::KCFI in rustc either AFAICT. Then later on it'd become more like:
config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC
def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG depends on RISCV || ((ARM64 || x86_64) && RUSTC_VERSION >= 107900) depends on (ARM64 || x86_64) || (RISCV && RUSTC_VERSION >= 999999) # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \ (!GCOV_KERNEL && !KASAN_GENERIC && !KASAN_SW_TAGS)
but that exact sort of mess is what becomes unwieldy fast since that doesn't even cover 32-bit arm.
Alice Ryhl
reviewerQuoted 140 lines · Conor Dooley
Quoted 94 lines · Alice Ryhl
Quoted 93 lines · Conor Dooley
Yo,
Quoted 40 lines · Miguel Ojeda
If KASAN is enabled, and one runs in a clean repository e.g.:
make LLVM=1 prepare
make LLVM=1 prepareThen the Rust code gets rebuilt, which should not happen.
The reason is some of the LLVM KASAN
rustcflags are added in the second run:-Cllvm-args=-asan-instrumentation-with-call-threshold=10000 -Cllvm-args=-asan-stack=0 -Cllvm-args=-asan-globals=1 -Cllvm-args=-asan-kernel-mem-intrinsic-prefix=1Further runs do not rebuild Rust because the flags do not change anymore.
Rebuilding like that in the second run is bad, even if this just happens with KASAN enabled, but missing flags in the first one is even worse.
The root issue is that we pass, for some architectures and for the moment, a generated
target.jsonfile. That file is not ready by the timerustcgets called for the flag test, and thus the flag test fails just because the file is not available, e.g.:$ ... --target=./scripts/target.json ... -Cllvm-args=... error: target file "./scripts/target.json" does not existThere are a few approaches we could take here to solve this. For instance, we could ensure that every time that the config is rebuilt, we regenerate the file and recompute the flags. Or we could use the LLVM version to check for these flags, instead of testing the flag (which may have other advantages, such as allowing us to detect renames on the LLVM side).
However, it may be easier than that:
rustcis aware of the-Cllvm-argsregardless of the--target(e.g. I checked that the list printed is the same, plus that I can check for these flags even if I pass a completely unrelated target), and thus we can just eliminate the dependency completely.Quoted 8 lines
Thus filter out the target.
This does mean that
rustc-optioncannot be used to test a flag that requires the right target, but we don't have other users yet, it is a minimal change and we want to get rid of custom targets in the future.Hmm, while this might be true, I think it should not actually have been true. Commit
ca627e636551e("rust: cfi: add support forCFI_CLANGwith Rust") added a cc-option check to the rust kconfig symbol, checking if the c compiler supports the integer normalisations stuff:depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)and also sets the relevant options in the makefile:
ifdefCONFIG_RUST
# Always pass -Zsanitizer-cfi-normalize-integers asCONFIG_RUSTselects #CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
RUSTC_FLAGS_CFI:= -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
KBUILD_RUSTFLAGS+= $(RUSTC_FLAGS_CFI)
exportRUSTC_FLAGS_CFI
endif
but it should also have added a rustc-option check as, unfortunately, support for kcfi in rustc is target specific. This results in build breakages where the arch supportsCFI_CLANGand RUST, but the target in use does not have the kcfi flag set.
I attempted to fix this by adding:diff --git a/arch/Kconfig b/arch/Kconfig index d1b4ffd6e0856..235709fb75152 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -916,6 +916,7 @@ config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG config HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC def_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG + depends on $(rustc-option,-C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers) depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \but of course this does not work for cross compilation, as you're
stripping the target information out and so the check passes on my host
even though my intended
RUSTC_BOOTSTRAP=1 rustc -C panic=abort -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers -Ctarget-cpu=generic-rv64 --target=riscv64imac-unknown-none-elf is a failure.I dunno too much about rustc itself, but I suspect that adding kcfi to the target there is a "free" win, but that'll take time to trickle down and the minimum version rustc version for the kernel isn't going to have that.
Quoted 7 lines
Quoted 3 lines
I'm not really sure what your target.json suggestion below is, so just reporting so that someone that understands the alternative solutions can fix this.
Probably right now we have to do this cfg by
depends on
CONFIG_ARMIt's valid on x86 too, right?
Quoted 5 lines
to prevent riscv if rustc has the missing setting set on riscv. Once we add it to riscv, we change it to
depends on CONFIG_ARM || (RUSTC_VERSION >= ??? || CONFIG_RISCV)I kinda shied away from something like this since there was already a cc-option on the other half and checking different versions per arch becomes a mess - but yeah it kinda is a no-brainer to do it here when rustc-option is kinda broken.
I guess the temporary fix is then:
config
HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTCdef_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG depends on ARM64 || x86_64 depends on RUSTC_VERSION >= 107900 # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \ (!GCOV_KERNEL && !KASAN_GENERIC && !KASAN_SW_TAGS)because there's no 32-bit target with
SanitizerSet::KCFIin rustc either AFAICT. Then later on it'd become more like:config
HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTCdef_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG depends on RISCV || ((ARM64 || x86_64) && RUSTC_VERSION >= 107900) depends on (ARM64 || x86_64) || (RISCV && RUSTC_VERSION >= 999999) # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \ (!GCOV_KERNEL && !KASAN_GENERIC && !KASAN_SW_TAGS)
Quoted 2 lines
but that exact sort of mess is what becomes unwieldy fast since that doesn't even cover 32-bit arm.
I think a better way of writing it is like this:
depends on ARCH1 || ARCH2 || ARCH3 depends on !ARCH1 || RUSTC_VERSION >= 000000 depends on !ARCH2 || RUSTC_VERSION >= 000000 depends on !ARCH3 || RUSTC_VERSION >= 000000
Alice
Quoted 14 lines
Quoted 11 lines
AFAICT. Then later on it'd become more like:
config
HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTCdef_bool y depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS_CLANG depends on RISCV || ((ARM64 || x86_64) && RUSTC_VERSION >= 107900) depends on (ARM64 || x86_64) || (RISCV && RUSTC_VERSION >= 999999) # With GCOV/KASAN we need this fix: https://github.com/rust-lang/rust/pull/129373 depends on (RUSTC_LLVM_VERSION >= 190103 && RUSTC_VERSION >= 108200) || \ (!GCOV_KERNEL && !KASAN_GENERIC && !KASAN_SW_TAGS)Quoted 2 lines
but that exact sort of mess is what becomes unwieldy fast since that doesn't even cover 32-bit arm.
Quoted 6 lines
I think a better way of writing it is like this:
depends on ARCH1 || ARCH2 || ARCH3 depends on !ARCH1 || RUSTC_VERSION >= 000000 depends on !ARCH2 || RUSTC_VERSION >= 000000 depends on !ARCH3 || RUSTC_VERSION >= 000000
Ye, that's a lot more manageable than what I came up with, shoulda really done better since the option I used for a reference looks a lot more like this than what I had... Thanks.
Quoted 4 lines · Conor Dooley
I'm not really sure what your target.json suggestion below is, so just reporting so that someone that understands the alternative solutions can fix this.
If you mean the last paragraph of the commit, then what I meant is that we could do the filter conditionally, so that rustc-option would work to test flags that require the target.json for those architectures that already do not use target.json.
Cheers,
Miguel