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: <Z8j8gwvnmKF9ZymM@pollux>
Date: Thu, 6 Mar 2025 02:38:11 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
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 06, 2025 at 01:27:19AM +0000, Benno Lossin wrote:
> 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)?

No, the compiler then explicitly complains that it expects a type.

> 
> > `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?

Didn't know that's possible (const _; () = { ... };). That's pretty nice, I will
go with my above proposal wrapped into the anonymous const. Thanks.