Skip to content

KASAN Support #1087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

KASAN Support #1087

wants to merge 2 commits into from

Conversation

maurer
Copy link

@maurer maurer commented Jul 9, 2024

Looking for a quick pre-review before sending these to the list

Right now, if we turn on KASAN, Rust code will cause violations because it's not enabled properly. Miguel previously gave us a patch that got this working by manually setting the right flags for current LLVM.

This PR:

  1. Adds flag probe macros for Rust - now that we're unforking alloc, these could be useful in general, and are needed here because we didn't set a restriction on how new of an LLVM rustc was using.
  2. Makes rustc enable the relevant sanitizer flags when C does.

maurer added 2 commits July 9, 2024 18:48
Creates flag probe macro variants for `rustc`. These are helpful
because:

1. `rustc` support will soon be a minimum rather than a pinned version.
2. We already support multiple LLVMs linked into `rustc`, and these are
   needed to probe what LLVM parameters `rustc` will accept.

Signed-off-by: Matthew Maurer <[email protected]>
Rust supports KASAN via LLVM, but prior to this patch, the flags aren't
set properly.

Suggested-by: Miguel Ojeda <[email protected]>
Signed-off-by: Matthew Maurer <[email protected]>
@ojeda
Copy link
Member

ojeda commented Jul 9, 2024

In case it is useful, I had this in a WIP branch -- I was concerned about flags being conditionally supported for some targets only.

diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
index 8fcb427405a6..d9505c83d099 100644
--- a/scripts/Makefile.compiler
+++ b/scripts/Makefile.compiler
@@ -20,6 +20,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
 # Exit code chooses option. "$$TMP" serves as a temporary file and is
 # automatically cleaned up.
 try-run = $(shell set -e;		\
+	TMPOUT=$(TMPOUT);		\
 	TMP=$(TMPOUT)/tmp;		\
 	trap "rm -rf $(TMPOUT)" EXIT;	\
 	mkdir -p $(TMPOUT);		\
@@ -61,6 +62,26 @@ cc-option-yn = $(call try-run,\
 cc-disable-warning = $(call try-run,\
 	$(CC) -Werror $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1)))
 
+# rustc-option
+#
+# Currently, this is only meant to be used by rustc-param, since we only support
+# a single Rust version for the time being.
+#
+# In addition, for the moment, we are passing here the base builtin target here
+# as an approximation of the actual target we will use, since in some cases we
+# still use the target.json spec, which is generated based on the configuration
+# (and thus it is not available where we would like to use rustc-option). When
+# we do not use target.json anymore, we should switch to pass KBUILD_RUSTFLAGS
+# which contains all the target-related flags, like it is done in cc-option.
+#
+# Moreover, please note that -Dwarnings currently does not convert every single
+# warning into an error [1], thus flags such as -Ctarget-feature cannot be
+# tested this way, but it does work for rustc-param.
+#
+# [1]: https://github.com/rust-lang/compiler-team/issues/473
+rustc-option = $(call try-run,\
+	echo '#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1 $(RUSTC) -Dwarnings --target=$(RUSTC_TARGET) $(1) --out-dir="$$TMPOUT" --emit=obj="$$TMP" --crate-type=lib -,$(1),$(2))
+
 # gcc-min-version
 # Usage: cflags-$(call gcc-min-version, 70100) += -foo
 gcc-min-version = $(call test-ge, $(CONFIG_GCC_VERSION), $1)
diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
index 390658a2d5b7..3159c2d2a7cb 100644
--- a/scripts/Makefile.kasan
+++ b/scripts/Makefile.kasan
@@ -12,6 +12,7 @@ endif
 KASAN_SHADOW_OFFSET ?= $(CONFIG_KASAN_SHADOW_OFFSET)
 
 cc-param = $(call cc-option, -mllvm -$(1), $(call cc-option, --param $(1)))
+rustc-param = $(call rustc-option, -Cllvm-args=-$(1))
 
 ifdef CONFIG_KASAN_STACK
 	stack_enable := 1

@ojeda
Copy link
Member

ojeda commented Jul 9, 2024

I see you use KBUILD_RUSTFLAGS, it would be nice to double-check the target.json interactions (i.e. for architectures that use it).

$(1) $(2) $(3) --crate-type=rlib /dev/null -o "$$TMP",$(3),$(4))

# rustc-option
# Usage: rustflags-y += $(call rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For flags that can be determined by the version, I wonder if it would be best to tell people to use rustc-min-version instead (I sent a (broken) patch adding that one and an example use case).

On one hand, having them versioned means we can easily grep & remove them when upgrading the compiler minimum.

On the other hand, it could be that someone has patched their compiler, e.g. backporting support for a given flag in a Linux distribution. So there is value, but I am not sure if it is just a theoretical advantage (and even then, a Linux distribution could also patch their kernel instead, too).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Of course, we still need it for rustc-param, that is what I did too -- but perhaps we can put a comment about it if we think one or the other way is better).

Copy link
Author

@maurer maurer Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in many cases rustc-min-version would be preferable, esp. because with -Z flags, the feature may be available but incomplete in some cases. However, I'd point out that users needing to look up what version of Rust added a flag they're going to use every time seems a bit more onerous than the situation in C.

I'll add a note to the docs that if you're testing for a -Z flag, you should consider rustc-min-version instead. I think rustc-option is probably better for -C options in most cases.

@ojeda
Copy link
Member

ojeda commented Jul 9, 2024

And if some of the above does not make sense, please ignore it -- I am just doing a brain dump here of things I was considering back then (a few months ago).

@maurer
Copy link
Author

maurer commented Jul 9, 2024

I see you use KBUILD_RUSTFLAGS, it would be nice to double-check the target.json interactions (i.e. for architectures that use it).

Yes, I'm now convinced that my current approach here is a bad idea, because availability of flags will vary based on targets.

I think what I'll do here is just remove this part from the patch - we should add support for this, but it doesn't need to be this patch, as we don't need it for rustc-param.

@maurer
Copy link
Author

maurer commented Jul 25, 2024

I see you use KBUILD_RUSTFLAGS, it would be nice to double-check the target.json interactions (i.e. for architectures that use it).

Yes, I'm now convinced that my current approach here is a bad idea, because availability of flags will vary based on targets.

I think what I'll do here is just remove this part from the patch - we should add support for this, but it doesn't need to be this patch, as we don't need it for rustc-param.

I take that back - KBUILD_RUSTFLAGS does have the appropriate target.json set, and x86_64 was the main arch I tested on, so I think this actually works as is.

@maurer
Copy link
Author

maurer commented Jul 26, 2024

Closing as it's been posted to the list for discussion.

@maurer maurer closed this Jul 26, 2024
ojeda pushed a commit that referenced this pull request Sep 16, 2024
Creates flag probe macro variants for `rustc`. These are helpful
because:

1. The kernel now supports a minimum `rustc` version rather than a
   single version.
2. `rustc` links against a range of LLVM revisions, occasionally even
   ones without an official release number. Since the availability of
   some Rust flags depends on which LLVM it has been linked against,
   probing is necessary.

Signed-off-by: Matthew Maurer <[email protected]>
Link: #1087
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Miguel Ojeda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants