Date:   Sat, 11 Feb 2023 15:35:33 +0100
From:   Andrea Righi <andrea.righi@canonical.com>
To:     Kees Cook <kees@kernel.org>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        bjorn3_gh@protonmail.com, Kees Cook <keescook@chromium.org>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] rust: allow to use INIT_STACK_ALL_ZERO
Message-ID: <Y+entVlMoV83vyzz@righiandr-XPS-13-7390>
References: <20230210215141.108958-1-andrea.righi@canonical.com>
 <8178319F-4188-4F8E-AD8D-DCC392E26DBF@kernel.org>
 <Y+daEOZgaiF3IMTX@righiandr-XPS-13-7390>
 <4E6FDE1C-47B6-46A6-AF31-28AEB2348344@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4E6FDE1C-47B6-46A6-AF31-28AEB2348344@kernel.org>
Precedence: bulk

On Sat, Feb 11, 2023 at 05:44:33AM -0800, Kees Cook wrote:
> On February 11, 2023 1:04:16 AM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
> >On Fri, Feb 10, 2023 at 02:43:56PM -0800, Kees Cook wrote:
> >> On February 10, 2023 1:51:41 PM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
> >> >With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
> >> >-ftrivial-auto-var-init=zero to clang, that triggers the following
> >> >error:
> >> >
> >> > error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'
> >> >
> >> >However, this additional option that is currently required by clang is
> >> >going to be removed in the future (as the name of the option suggests),
> >> >likely with clang-17.
> >> >
> >> >So, make sure bindgen is using this extra option if the major version of
> >> >the libclang used by bindgen is < 17.
> >> >
> >> >In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
> >> >without triggering any build error.
> >> >
> >> >Link: https://github.com/llvm/llvm-project/issues/44842
> >> >Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> >> >---
> >> >
> >> >Changes in v2:
> >> > - check the version of libclang used by bindgen to determine if we need
> >> >   to pass the additional clang option
> >> >
> >> > rust/Makefile | 13 +++++++++++++
> >> > 1 file changed, 13 insertions(+)
> >> >
> >> >diff --git a/rust/Makefile b/rust/Makefile
> >> >index ff70c4c916f8..c77d7ce96a85 100644
> >> >--- a/rust/Makefile
> >> >+++ b/rust/Makefile
> >> >@@ -269,6 +269,19 @@ BINDGEN_TARGET		:= $(BINDGEN_TARGET_$(SRCARCH))
> >> > # some configurations, with new GCC versions, etc.
> >> > bindgen_extra_c_flags = -w --target=$(BINDGEN_TARGET)
> >> > 
> >> >+# Auto variable zero-initialization requires an additional special option with
> >> >+# clang that is going to be removed sometimes in the future (likely in
> >> >+# clang-17), so make sure to pass this option only if clang supports it
> >> >+# (libclang major version < 17).
> >> >+#
> >> >+# https://github.com/llvm/llvm-project/issues/44842
> >> >+ifdef CONFIG_INIT_STACK_ALL_ZERO
> >> >+libclang_maj_ver=$(shell $(BINDGEN) $(srctree)/scripts/rust_is_available_bindgen_libclang.h 2>&1 | sed -ne 's/.*clang version \([0-9]*\).*/\1/p')
> >> >+ifeq ($(shell expr $(libclang_maj_ver) \< 17), 1)
> >> >+bindgen_extra_c_flags += -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> >> >+endif
> >> >+endif
> >> 
> >> This logic already exists in the top-level Makefile. How does -ftrivial-auto-var-init=zero make it into bindgen_c_flags normally? (I.e. why does the legacy -enable... option not?)
> >
> >If we're using gcc, the top-level Makefile doesn't set the clang
> >options, so in this case CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO_ENABLER is
> >undefined.  But then bindgen always relies on libclang to parse C, even
> >if gcc is used globally, therefore it needs the extra clang flag.
> 
> Ah yes! Thank you, I keep forgetting about mixed compiler builds.
> 
> >
> >Ideally it'd be nice if bindgen would support a gcc backend, but until
> >then we need to do something special here, like this kind of duplicated
> >logic...
> 
> Right. Yeah, good fix. One nit: the -enable... flag is removed in Clang 16+:

oic, with clang-16 we get a deprecated warning (but it doesn't fail),
then starting with clang-17 the -enable.. flag is not available anymore.

Then I agree that a better check would be version < 16, instead of < 17.

Thanks,
-Andrea

> 
> https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags
> 
> With that fixed:
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> 
> -- 
> Kees Cook