linux-kernel - Re: [PATCH v3 5/5] rust: Add warn_on and warn_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20250305.192403.996225631653343672.fujita.tomonori@gmail.com>
Date: Wed, 05 Mar 2025 19:24:03 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: aliceryhl@...gle.com
Cc: fujita.tomonori@...il.com, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, x86@...nel.org,
 linux-riscv@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 loongarch@...ts.linux.dev, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com, peterz@...radead.org,
 hpa@...or.com, paul.walmsley@...ive.com, palmer@...belt.com,
 aou@...s.berkeley.edu, catalin.marinas@....com, will@...nel.org,
 chenhuacai@...nel.org, kernel@...0n.name, tangyouling@...ngson.cn,
 hejinyang@...ngson.cn, yangtiezhu@...ngson.cn, ojeda@...nel.org,
 alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
 bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, a.hindborg@...nel.org,
 tmgross@...ch.edu
Subject: Re: [PATCH v3 5/5] rust: Add warn_on and warn_on_once

On Wed, 5 Mar 2025 08:42:57 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> On Thu, Feb 13, 2025 at 10:57:59PM +0900, FUJITA Tomonori wrote:
>> Add warn_on and warn_on_once macros. Wrapping the C's WARN_* and BUG_*
>> macros doesn't work so this uses the assembly code exported by the C
>> side via ARCH_WARN_ASM macro. Like the static branch code, this
>> generates the assembly code for rust at compile time by using the C
>> preprocessor.
>> 
>> file()! macro doesn't work for the Rust inline assembly in the same
>> way as __FILE__ for the C inline assembly. So the code to handle a
>> file name is different from the C assembly code (similar to the
>> arm64/loongarch assembly).
> 
> Nit: Should be file!() not file()!.

Ops, thanks.

Actually, the above comment is obsolete. With your solution in the
previous mail, I can remove the asm code for the file name. I'll
remove the comment.


>> diff --git a/rust/kernel/.gitignore b/rust/kernel/.gitignore
>> index 6ba39a178f30..f1d7f4225332 100644
>> --- a/rust/kernel/.gitignore
>> +++ b/rust/kernel/.gitignore
>> @@ -1,3 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>>  /generated_arch_static_branch_asm.rs
>> +/generated_arch_warn_asm.rs
>> +/generated_arch_reachable_asm.rs
>> \ No newline at end of file
> 
> There should be a newline.

Ah, I'll fix.

>> +++ b/rust/kernel/bug.rs
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2024 FUJITA Tomonori
> 
> 2025?

I'll add.

>> +#[macro_export]
>> +#[doc(hidden)]
>> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
>> +macro_rules! warn_flags {
>> +    ($flags:expr) => {
>> +        const FLAGS: u32 = $crate::bindings::BUGFLAG_WARNING | $flags;
>> +        // SAFETY: Just an FFI call.
>> +        #[cfg(CONFIG_DEBUG_BUGVERBOSE)]
>> +        unsafe {
>> +            $crate::asm!(concat!(
>> +                "/* {size} */",
>> +                ".pushsection .rodata.str1.1, \"aMS\",@progbits, 1\n",
>> +                "111:\t .string ", "\"", file!(), "\"\n",
>> +                ".popsection\n",
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_warn_asm.rs")),
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_reachable_asm.rs")));
>> +            line = const line!(),
>> +            flags = const FLAGS,
>> +            size = const ::core::mem::size_of::<$crate::bindings::bug_entry>(),
>> +            );
>> +        }
>> +        // SAFETY: Just an FFI call.
>> +        #[cfg(not(CONFIG_DEBUG_BUGVERBOSE))]
>> +        unsafe {
>> +            $crate::asm!(
>> +            concat!(
>> +                "/* {size} */",
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_warn_asm.rs")),
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_reachable_asm.rs")));
>> +            flags = const FLAGS,
>> +            size = const ::core::mem::size_of::<$crate::bindings::bug_entry>(),
>> +            );
>> +        }
> 
> I generally prefer to have the cfgs on the macro rather in its
> expansion. That avoids emitting a lot of code that is not actually used.

You prefer the following?

#[cfg(all(CONFIG_BUG, CONFIG_DEBUG_BUGVERBOSE, not(CONFIG_UML)))]
macro_rules! warn_flags {
...
}

#[cfg(all(CONFIG_BUG, not(CONFIG_DEBUG_BUGVERBOSE), not(CONFIG_UML)))]
macro_rules! warn_flags {
...
}

>> +#[doc(hidden)]
>> +#[macro_export]
>> +macro_rules! bugflag_taint {
>> +    ($taint:expr) => {
>> +        $taint << 8
>> +    };
>> +}
> 
> This could just be a const fn.

Yeah, would a const fn be preferable?

>> +/// Report a warning only once.
>> +#[macro_export]
>> +macro_rules! warn_on_once {
>> +    ($cond:expr) => {
>> +        if $cond {
>> +            $crate::warn_flags!(
>> +                $crate::bindings::BUGFLAG_ONCE
>> +                    | $crate::bugflag_taint!($crate::bindings::TAINT_WARN)
> 
> Or maybe a constant?
> 
> const WARN_ON_ONCE_FLAGS: u32 = bindings::BUGFLAG_ONCE | (bindings::TAINT_WARN << 8);

Ok, but you prefer "<< 8" than using const fn bugflag_taint()?

> $crate::warn_flags!($crate::bug::WARN_ON_ONCE_FLAGS);