netdev - Re: [PATCH net-next v6 2/5] rust: net::phy add 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]

Date: Wed, 25 Oct 2023 07:29:32 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v6 2/5] rust: net::phy add module_phy_driver macro

On 25.10.23 02:02, FUJITA Tomonori wrote:
> On Tue, 24 Oct 2023 16:28:02 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
> 
>> On 24.10.23 02:58, FUJITA Tomonori wrote:
>>> This macro creates an array of kernel's `struct phy_driver` and
>>> registers it. This also corresponds to the kernel's
>>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
>>> loading into the module binary file.
>>>
>>> A PHY driver should use this macro.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>>> ---
>>>    rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 129 insertions(+)
>>>
>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>>> index 2d821c2475e1..f346b2b4d3cb 100644
>>> --- a/rust/kernel/net/phy.rs
>>> +++ b/rust/kernel/net/phy.rs
>>> @@ -706,3 +706,132 @@ const fn as_int(&self) -> u32 {
>>>            }
>>>        }
>>>    }
>>> +
>>> +/// Declares a kernel module for PHYs drivers.
>>> +///
>>> +/// This creates a static array of kernel's `struct phy_driver` and registers it.
>>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
>>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```ignore
>>
>> Is this example ignored, because it does not compile?
> 
> The old version can't be compiled but the current version can so I'll
> drop ignore.
> 
> 
>> I think Wedson was wrapping his example with `module!` inside
>> of a module, so maybe try that?
> 
> I'm not sure what you mean.

Wedson did this [1], note the `# mod module_fs_sample`:

/// # Examples
///
/// ```
/// # mod module_fs_sample {
/// use kernel::prelude::*;
/// use kernel::{c_str, fs};
///
/// kernel::module_fs! {
///     type: MyFs,
///     name: "myfs",
///     author: "Rust for Linux Contributors",
///     description: "My Rust fs",
///     license: "GPL",
/// }
///
/// struct MyFs;
/// impl fs::FileSystem for MyFs {
///     const NAME: &'static CStr = c_str!("myfs");
/// }
/// # }
/// ```

[1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124

>>> +/// use kernel::c_str;
>>> +/// use kernel::net::phy::{self, DeviceId};
>>> +/// use kernel::prelude::*;

[...]

>>> +        const _: () = {
>>> +            static mut DRIVERS: [
>>> +                ::kernel::net::phy::DriverVTable;
>>> +                $crate::module_phy_driver!(@count_devices $($driver),+)
>>> +            ] = [
>>> +                $(::kernel::net::phy::create_phy_driver::<$driver>()),+
>>> +            ];
>>> +
>>> +            impl ::kernel::Module for Module {
>>> +                fn init(module: &'static 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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?;
>>
>> Can you put the safe operations outside of the `unsafe` block?
> 
> fn init(module: &'static 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 mut reg = ::kernel::net::phy::Registration::register(
>          module,
>          core::pin::Pin::static_mut(unsafe { &mut DRIVERS }),
>      )?;
>      Ok(Module { _reg: reg })
> }

Here the `unsafe` block and the `SAFETY` comment are pretty far away.
What about this?:

fn init(module: &'static 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 };
     let mut reg =
         ::kernel::net::phy::Registration::register(module, ::core::pin::Pin::static_mut(drivers))?;
     Ok(Module { _reg: reg })
}

Also note that I added `::` to the front of `core`.

-- 
Cheers,
Benno