netdev - Re: [PATCH net] rust: net::phy scope ThisModule usage in the module_phy

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: <CAH5fLgiFFhrDd+=+0gsnaWdV=EeiExo3SQxg2=2c3m-5Z5Tgqg@mail.gmail.com>
Date: Mon, 25 Nov 2024 09:59:13 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Rahul Rameshbabu <sergeantsagara@...tonmail.com>, rust-for-linux@...r.kernel.org, 
	netdev@...r.kernel.org, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	Trevor Gross <tmgross@...ch.edu>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] rust: net::phy scope ThisModule usage in the
 module_phy_driver macro

On Mon, Nov 25, 2024 at 1:27 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 13 Nov 2024 17:45:22 +0000 Rahul Rameshbabu wrote:
> > Similar to the use of $crate::Module, ThisModule should be referred to as
> > $crate::ThisModule in the macro evaluation. The reason the macro previously
> > did not cause any errors is because all the users of the macro would use
> > kernel::prelude::*, bringing ThisModule into scope.
>
> You say "previously", does it mean there are no in-tree users where
> this could cause bugs? If so no Fixes tag necessary..
>
> > Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
> > Signed-off-by: Rahul Rameshbabu <sergeantsagara@...tonmail.com>
> > ---
> >
> > Notes:
> >     How I came up with this change:
> >
> >     I was working on my own rust bindings and rust driver when I compared my
> >     macro_rule to the one used for module_phy_driver. I noticed, if I made a
> >     driver that does not use kernel::prelude::*, that the ThisModule type
> >     identifier used in the macro would cause an error without being scoped in
> >     the macro_rule. I believe the correct implementation for the macro is one
> >     where the types used are correctly expanded with needed scopes.
>
> Rust experts, does the patch itself make sense?

Yes, the macro should not rely on the user having random things in
scope when calling the macro. This change is good.

Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>

> > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> > index 910ce867480a..80f9f571b88c 100644
> > --- a/rust/kernel/net/phy.rs
> > +++ b/rust/kernel/net/phy.rs
> > @@ -837,7 +837,7 @@ const fn as_int(&self) -> u32 {
> >  ///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
> >  ///
> >  ///     impl ::kernel::Module for Module {
> > -///         fn init(module: &'static ThisModule) -> Result<Self> {
> > +///         fn init(module: &'static ::kernel::ThisModule) -> Result<Self> {
> >  ///             let drivers = unsafe { &mut DRIVERS };
> >  ///             let mut reg = ::kernel::net::phy::Registration::register(
> >  ///                 module,
> > @@ -899,7 +899,7 @@ struct Module {
> >                  [$($crate::net::phy::create_phy_driver::<$driver>()),+];
> >
> >              impl $crate::Module for Module {
> > -                fn init(module: &'static ThisModule) -> Result<Self> {
> > +                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
> >                      // SAFETY: The anonymous constant guarantees that nobody else can access
> >                      // the `DRIVERS` static. The array is used only in the C side.
> >                      let drivers = unsafe { &mut DRIVERS };
> >
> > base-commit: 73af53d82076bbe184d9ece9e14b0dc8599e6055
>