These flags are not made conditional on compiler support because at the
moment exactly one version of rustc supported, and that one supports
these flags.

Building without these additional flags will manifest as objtool
printing a large number of errors about missing ENDBR and if CFI is
enabled (not currently possible) will result in incorrectly structured
function prefixes.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---

Split out the IBT additions as per
https://lkml.kernel.org/linux-fsdevel/CANiq72kK6ppBE7j=z7uua1cJMKaLoR5U3NUAZXT5MrNEs9ZhfQ@mail.gmail.com/

arch/x86/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 5bfe5caaa444..941f7abf6dbf 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -81,6 +81,7 @@ ifeq ($(CONFIG_X86_KERNEL_IBT),y)
 #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104816
 #
 KBUILD_CFLAGS += $(call cc-option,-fcf-protection=branch -fno-jump-tables)
+KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
 else
 KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none)
 endif
-- 
2.42.0.609.gbb76f46606-goog

On Mon, Oct 9, 2023 at 6:44 PM Matthew Maurer <mmaurer@google.com> wrote:
> +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables

I have not tested this, but is it possible to enable these options via
`-Cllvm-args=...` instead of using the unstable flags?

If so, I think this would be preferred in case the exact flags change
before they become stable. It sounds like they are likely to change,
see [1].

If not, no big deal since it would just need an update at a rust version bump.

- Trevor

[1]: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Zbranch-protection.60.20stability

On Thu, Oct 12, 2023 at 10:13 PM Trevor Gross <tmgross@umich.edu> wrote:
>
> I have not tested this, but is it possible to enable these options via
> `-Cllvm-args=...` instead of using the unstable flags?

We probably want to use the "real" flag eventually instead of
`-Cllvm-args`, right? So we would need to change it anyhow. And using
the `-Z` one means we test the "real" flag already.

Well, unless `-Cllvm-args` becomes the "official" way to enable this,
like you suggest in the Zulip, but should that really happen? e.g.
should not there be a generic flag for all backends for things like
these?

> If so, I think this would be preferred in case the exact flags change
> before they become stable. It sounds like they are likely to change,
> see [1].

That is fine, they will change anyway from `-Z` to `-C`, so having to
update those is expected.

> If not, no big deal since it would just need an update at a rust version bump.

Yeah, I don't think it is a big deal, and the version bump looks like
the best commit to put the change, in fact.

It is true, though, that these ones in particular are conditionally
enabled, so there is a slightly higher risk of forgetting about them.
But that is why we should get more `Tested-by`s! :)

Cheers,
Miguel

On Tue, Oct 10, 2023 at 12:43 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> These flags are not made conditional on compiler support because at the
> moment exactly one version of rustc supported, and that one supports
> these flags.
>
> Building without these additional flags will manifest as objtool
> printing a large number of errors about missing ENDBR and if CFI is
> enabled (not currently possible) will result in incorrectly structured
> function prefixes.
>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>
> Split out the IBT additions as per
> https://lkml.kernel.org/linux-fsdevel/CANiq72kK6ppBE7j=z7uua1cJMKaLoR5U3NUAZXT5MrNEs9ZhfQ@mail.gmail.com/

Thanks a lot Matthew for this! It is great to see those warnings
finally go away.

I have added the `objtool` pass to the intermediate Rust object files
and, with that + this patch applied + IBT enabled (but not
rethunk/retpoline), the only thing I see is:

    samples/rust/rust_print.o: warning: objtool: init_module(): not an
indirect call target
    samples/rust/rust_print.o: warning: objtool: cleanup_module(): not
an indirect call target

But we can fix those independently of this (ideally we want to reuse
the C macros, rather than putting more complexity in `module!`), so:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

I will send the patch for adding `objtool`.

Cheers,
Miguel

On Mon, Oct 09, 2023 at 10:42:54PM +0000, Matthew Maurer wrote:
> These flags are not made conditional on compiler support because at the
> moment exactly one version of rustc supported, and that one supports
> these flags.
> 
> Building without these additional flags will manifest as objtool
> printing a large number of errors about missing ENDBR and if CFI is
> enabled (not currently possible) will result in incorrectly structured
> function prefixes.

Well, I would also imagine running it on actual IBT enabled hardware
will get you a non-booting kernel.

> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
> 
> Split out the IBT additions as per
> https://lkml.kernel.org/linux-fsdevel/CANiq72kK6ppBE7j=z7uua1cJMKaLoR5U3NUAZXT5MrNEs9ZhfQ@mail.gmail.com/
> 
> arch/x86/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 5bfe5caaa444..941f7abf6dbf 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -81,6 +81,7 @@ ifeq ($(CONFIG_X86_KERNEL_IBT),y)
>  #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104816
>  #
>  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=branch -fno-jump-tables)
> +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables

One question, -Zcf-protection=branch, will that ever emit NOTRACK
prefix? The kernel very explicitly does not support (enable) NOTRACK.

>  else
>  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none)
>  endif
> -- 
> 2.42.0.609.gbb76f46606-goog
>

On Tue, Oct 10, 2023 at 1:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Oct 09, 2023 at 10:42:54PM +0000, Matthew Maurer wrote:
> > These flags are not made conditional on compiler support because at the
> > moment exactly one version of rustc supported, and that one supports
> > these flags.
> >
> > Building without these additional flags will manifest as objtool
> > printing a large number of errors about missing ENDBR and if CFI is
> > enabled (not currently possible) will result in incorrectly structured
> > function prefixes.
>
> Well, I would also imagine running it on actual IBT enabled hardware
> will get you a non-booting kernel.

Do you know what machine type in QEMU first supports IBT?

>
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> > https://godbolt.org/z/bc4n6sq5q

Intel asm syntax...my eyes!!!

> > ---
> >
> > Split out the IBT additions as per
> > https://lkml.kernel.org/linux-fsdevel/CANiq72kK6ppBE7j=z7uua1cJMKaLoR5U3NUAZXT5MrNEs9ZhfQ@mail.gmail.com/
> >
> > arch/x86/Makefile | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > index 5bfe5caaa444..941f7abf6dbf 100644
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -81,6 +81,7 @@ ifeq ($(CONFIG_X86_KERNEL_IBT),y)
> >  #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104816
> >  #
> >  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=branch -fno-jump-tables)
> > +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
>
> One question, -Zcf-protection=branch, will that ever emit NOTRACK
> prefix? The kernel very explicitly does not support (enable) NOTRACK.
>
> >  else
> >  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none)
> >  endif
> > --
> > 2.42.0.609.gbb76f46606-goog
> >
>


-- 
Thanks,
~Nick Desaulniers

On Tue, Oct 10, 2023 at 11:39 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> > > https://godbolt.org/z/bc4n6sq5q
>
> Intel asm syntax...my eyes!!!

:) If it helps, you can click on the "output" (gear) dropdown in any
of the asm panels and toggle between Intel and AT&T

On Tue, Oct 10, 2023 at 08:38:58AM -0700, Nick Desaulniers wrote:
> On Tue, Oct 10, 2023 at 1:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Oct 09, 2023 at 10:42:54PM +0000, Matthew Maurer wrote:
> > > These flags are not made conditional on compiler support because at the
> > > moment exactly one version of rustc supported, and that one supports
> > > these flags.
> > >
> > > Building without these additional flags will manifest as objtool
> > > printing a large number of errors about missing ENDBR and if CFI is
> > > enabled (not currently possible) will result in incorrectly structured
> > > function prefixes.
> >
> > Well, I would also imagine running it on actual IBT enabled hardware
> > will get you a non-booting kernel.
> 
> Do you know what machine type in QEMU first supports IBT?

I'm not sure QEMU has IBT support at all atm -- the whole CET
virtualization stuff is a bit of a mess.

But hardware wise it's Tigerlake (11) and everything after that - so
Alderlake (12) and Saphire Rapids (4th gen scalable -- or somesuch
nonsense).

On Tue, Oct 10, 2023 at 1:12 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Oct 09, 2023 at 10:42:54PM +0000, Matthew Maurer wrote:
> > These flags are not made conditional on compiler support because at the
> > moment exactly one version of rustc supported, and that one supports
> > these flags.
> >
> > Building without these additional flags will manifest as objtool
> > printing a large number of errors about missing ENDBR and if CFI is
> > enabled (not currently possible) will result in incorrectly structured
> > function prefixes.
>
> Well, I would also imagine running it on actual IBT enabled hardware
> will get you a non-booting kernel.
>
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > ---
> >
> > Split out the IBT additions as per
> > https://lkml.kernel.org/linux-fsdevel/CANiq72kK6ppBE7j=z7uua1cJMKaLoR5U3NUAZXT5MrNEs9ZhfQ@mail.gmail.com/
> >
> > arch/x86/Makefile | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > index 5bfe5caaa444..941f7abf6dbf 100644
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -81,6 +81,7 @@ ifeq ($(CONFIG_X86_KERNEL_IBT),y)
> >  #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104816
> >  #
> >  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=branch -fno-jump-tables)
> > +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
>
> One question, -Zcf-protection=branch, will that ever emit NOTRACK
> prefix? The kernel very explicitly does not support (enable) NOTRACK.
rustc does this via LLVM, so its code generation works very similarly to clang.
It does not create its own explicit NOTRACKs, but LLVM will by default
with just -Zcf-protection-branch.
I've linked a godbolt showing that at least for the basic case, your
no-jump-tables approach from clang ports over.
https://godbolt.org/z/bc4n6sq5q
Whether rust generates NOTRACK should end up being roughly equivalent
to whether clang generates it, and if LLVM gains a code generation
flag for NOTRACK being disallowed some day, we can pass that through
as well.
>
> >  else
> >  KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none)
> >  endif
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

On Tue, Oct 10, 2023 at 07:06:32AM -0700, Matthew Maurer wrote:

> > > +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
> >
> > One question, -Zcf-protection=branch, will that ever emit NOTRACK
> > prefix? The kernel very explicitly does not support (enable) NOTRACK.

> rustc does this via LLVM, so its code generation works very similarly to clang.
> It does not create its own explicit NOTRACKs, but LLVM will by default
> with just -Zcf-protection-branch.
> I've linked a godbolt showing that at least for the basic case, your
> no-jump-tables approach from clang ports over.
> https://godbolt.org/z/bc4n6sq5q
> Whether rust generates NOTRACK should end up being roughly equivalent
> to whether clang generates it, and if LLVM gains a code generation
> flag for NOTRACK being disallowed some day, we can pass that through
> as well.

IIRC C++ will also emit NOTRACK for things like catch/throw and other
stack/scope unwinds. Obviously C doesn't have that, but does Rust? (as
might be obvious, I *really* don't know the language).

ISTR HJL had a GCC patch to force-disable NOTRACK, but I've no idea what
happened to that.

On Tue, Oct 10, 2023 at 7:24 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 10, 2023 at 07:06:32AM -0700, Matthew Maurer wrote:
>
> > > > +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
> > >
> > > One question, -Zcf-protection=branch, will that ever emit NOTRACK
> > > prefix? The kernel very explicitly does not support (enable) NOTRACK.
>
> > rustc does this via LLVM, so its code generation works very similarly to clang.
> > It does not create its own explicit NOTRACKs, but LLVM will by default
> > with just -Zcf-protection-branch.
> > I've linked a godbolt showing that at least for the basic case, your
> > no-jump-tables approach from clang ports over.
> > https://godbolt.org/z/bc4n6sq5q
> > Whether rust generates NOTRACK should end up being roughly equivalent
> > to whether clang generates it, and if LLVM gains a code generation
> > flag for NOTRACK being disallowed some day, we can pass that through
> > as well.
>
> IIRC C++ will also emit NOTRACK for things like catch/throw and other
> stack/scope unwinds. Obviously C doesn't have that, but does Rust? (as
> might be obvious, I *really* don't know the language).
>
That's fine - Rust does have stack/scope unwinds with the
`panic=unwind` strategy. In the kernel, we use `panic=abort` and are
unlikely to ever change this approach. There are a host of other
complications that come from unwinding without NOTRACK getting
involved :)

In case you find `catch_unwind` - this function only has an effect
with `panic=unwind`. When `panic=abort`, there's nothing analogous to
catch/throw anymore, and `catch_unwind` becomes a no-op.

Are there other features you expect might trigger NOTRACK?
> ISTR HJL had a GCC patch to force-disable NOTRACK, but I've no idea what
> happened to that.
>

On Tue, Oct 10, 2023 at 07:34:45AM -0700, Matthew Maurer wrote:
> On Tue, Oct 10, 2023 at 7:24 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Oct 10, 2023 at 07:06:32AM -0700, Matthew Maurer wrote:
> >
> > > > > +KBUILD_RUSTFLAGS += -Zcf-protection=branch -Zno-jump-tables
> > > >
> > > > One question, -Zcf-protection=branch, will that ever emit NOTRACK
> > > > prefix? The kernel very explicitly does not support (enable) NOTRACK.
> >
> > > rustc does this via LLVM, so its code generation works very similarly to clang.
> > > It does not create its own explicit NOTRACKs, but LLVM will by default
> > > with just -Zcf-protection-branch.
> > > I've linked a godbolt showing that at least for the basic case, your
> > > no-jump-tables approach from clang ports over.
> > > https://godbolt.org/z/bc4n6sq5q
> > > Whether rust generates NOTRACK should end up being roughly equivalent
> > > to whether clang generates it, and if LLVM gains a code generation
> > > flag for NOTRACK being disallowed some day, we can pass that through
> > > as well.
> >
> > IIRC C++ will also emit NOTRACK for things like catch/throw and other
> > stack/scope unwinds. Obviously C doesn't have that, but does Rust? (as
> > might be obvious, I *really* don't know the language).
> >
> That's fine - Rust does have stack/scope unwinds with the
> `panic=unwind` strategy. In the kernel, we use `panic=abort` and are
> unlikely to ever change this approach. There are a host of other
> complications that come from unwinding without NOTRACK getting
> involved :)
> 
> In case you find `catch_unwind` - this function only has an effect
> with `panic=unwind`. When `panic=abort`, there's nothing analogous to
> catch/throw anymore, and `catch_unwind` becomes a no-op.
> 
> Are there other features you expect might trigger NOTRACK?

I'm not sure -- if they happen, objtool should warn about them. So I
suppose we'll take it from there.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

On Tue, Oct 10, 2023 at 4:56 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> I'm not sure -- if they happen, objtool should warn about them. So I
> suppose we'll take it from there.
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Applied to `rust-next` -- thanks!

For subsequent patches, do you want that they go through the tip tree?
If so, please let me know, I think it would be ideal.

Thanks!

Cheers,
Miguel