linux-kernel - Re: [PATCH v5 3/5] rust: firmware: add `module

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D88SJOTH9GN4.3OVO4JFYAF9R2@proton.me>
Date: Thu, 06 Mar 2025 01:27:19 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
Cc: airlied@...il.com, simona@...ll.ch, corbet@....net, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, ajanulgu@...hat.com, lyude@...hat.com, pstanner@...hat.com, zhiw@...dia.com, cjia@...dia.com, jhubbard@...dia.com, bskeggs@...dia.com, acurrid@...dia.com, ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu, gregkh@...uxfoundation.org, mcgrof@...nel.org, russ.weight@...ux.dev, dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
>> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>>
>> > +#[macro_export]
>> > +macro_rules! module_firmware {
>> > +    ($($builder:tt)*) => {
>>
>> This should probably be `$builder:expr` instead.
>
> That doesn't work, the compiler then complains, since it's not an expression:
>
> 193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
>      |                                                ^^ expected one of `.`, `?`, `]`, or an operator

Does `<$builder>::create` work (with the `expr` fragment)?

> `ty` doesn't work either, since then the compiler expects the caller to add the
> const generic, which we want the macro to figure out instead.
>
>>
>> > +
>> > +        #[cfg(not(MODULE))]
>> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> > +            <LocalModule as kernel::ModuleMetadata>::NAME
>>
>> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
>
> Good catch, thanks.
>
>>
>> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
>> the current module for some reason, no idea if there is a rational
>> argument behind that, but it just doesn't sit right with me.
>>
>> Essentially you're doing this for convenience, right? So you don't want
>> to have to repeat the name of the module type every time?
>
> No, it's really that I can't know the type name here, please see the previous
> patch commit message that introduces `LocalModule` for explanation.

Gotcha.

>> > +        }
>> > +
>> > +        #[cfg(MODULE)]
>> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> > +            kernel::c_str!("")
>>
>> Ditto.
>>
>> > +        }
>>
>> Are these two functions used outside of the `static` below? If no, then
>> you can just move them into the static? You can also probably use a
>> `const` instead of a function, that way you only have 4 lines instead
>> of 8.
>
> Is this what you're proposing?
>
> 	#[macro_export]
> 	macro_rules! module_firmware {
> 	    ($($builder:tt)*) => {
> 	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> 	            $crate::c_str!("")
> 	        } else {
> 	            <LocalModule as $crate::ModuleMetadata>::NAME
> 	        };
>
> 	        #[link_section = ".modinfo"]
> 	        #[used]
> 	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
> 	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();

I meant to also move the `const` into the expression, but I guess that
leads to duplication:

    #[link_section = ".modinfo"]
    #[used]
    static __MODULE_FIRMWARE: [u8; {
        const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
            $crate::c_str!("")
        } else {
            <LocalModule as $crate::ModuleMetadata>::NAME
        };
        <$builder>::create(PREFIX).build_length()
    }] = {
        const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
            $crate::c_str!("")
        } else {
            <LocalModule as $crate::ModuleMetadata>::NAME
        };
        <$builder>::create(PREFIX)
    };

But then the advantage is that only the `__MODULE_FIRMWARE` static will
be in-scope.

Do you think that its useful to have the static be accessible? I.e. do
users need to access it (I would think they don't)? If they don't, then
we could put all of those things into a `const _: () = { /* ... */ };`.
But then people can invoke `module_firmware!` multiple times in the same
module, is that a problem?

---
Cheers,
Benno

> 	    };
> 	}