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]

Message-Id: <20231025.165710.1134967889825495180.fujita.tomonori@gmail.com>
Date: Wed, 25 Oct 2023 16:57:10 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, 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 Wed, 25 Oct 2023 07:29:32 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> 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

You are suggesting like the following?

/// # Examples
///
/// ```
/// # mod module_phy_driver_sample {
...
/// # }
/// ```

What benefits?


>>>> +        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`.

I see.