netdev - Re: [net-next PATCH v11 1/6] net: phy: pass PHY driver to .match_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: <20250517.222804.482303667530450320.fujita.tomonori@gmail.com>
Date: Sat, 17 May 2025 22:28:04 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: lossin@...nel.org
Cc: ansuelsmth@...il.com, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, hkallweit1@...il.com,
 linux@...linux.org.uk, florian.fainelli@...adcom.com,
 bcm-kernel-feedback-list@...adcom.com, kabel@...nel.org,
 andrei.botila@....nxp.com, fujita.tomonori@...il.com, tmgross@...ch.edu,
 ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...nel.org, aliceryhl@...gle.com, dakr@...nel.org,
 sd@...asysnail.net, michael@...sekall.de, daniel@...rotopia.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 rmk+kernel@...linux.org.uk
Subject: Re: [net-next PATCH v11 1/6] net: phy: pass PHY driver to
 .match_phy_device OP

On Sat, 17 May 2025 10:09:53 +0200
"Benno Lossin" <lossin@...nel.org> wrote:

> On Fri May 16, 2025 at 11:23 PM CEST, Christian Marangi wrote:
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index a59469c785e3..079a0f884887 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -418,15 +418,18 @@ impl<T: Driver> Adapter<T> {
>>  
>>      /// # Safety
>>      ///
>> -    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
>> +    /// `phydev` and `phydrv` must be passed by the corresponding callback in
>> +    //  `phy_driver`.
>>      unsafe extern "C" fn match_phy_device_callback(
>>          phydev: *mut bindings::phy_device,
>> +        phydrv: *const bindings::phy_driver,
>>      ) -> crate::ffi::c_int {
>>          // SAFETY: This callback is called only in contexts
>>          // where we hold `phy_device->lock`, so the accessors on
>>          // `Device` are okay to call.
>>          let dev = unsafe { Device::from_raw(phydev) };
>> -        T::match_phy_device(dev) as i32
>> +        let drv = unsafe { T::from_raw(phydrv) };
>> +        T::match_phy_device(dev, drv) as i32
>>      }
>>  
>>      /// # Safety
>> @@ -574,6 +577,19 @@ pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
>>  /// This trait is used to create a [`DriverVTable`].
>>  #[vtable]
>>  pub trait Driver {
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of `'a`, the pointer must point at a valid
>> +    /// `phy_driver`, and the caller must be in a context where all
>> +    /// methods defined on this struct are safe to call.
>> +    unsafe fn from_raw<'a>(ptr: *const bindings::phy_driver) -> &'a DriverVTable {
>> +        // CAST: `DriverVTable` is a `repr(transparent)` wrapper around `bindings::phy_driver`.
>> +        let ptr = ptr.cast::<DriverVTable>();
>> +        // SAFETY: by the function requirements the pointer is const and is
>> +        // always valid to access for the duration of `'a`.
>> +        unsafe { &*ptr }
>> +    }
> 
> If we go the way of supplying a `&DriverVTable` in the
> `match_phy_device` function, then this should be a function in the impl
> block of `DriverVTable` and not in `Driver`.

Yeah.

> See my reply to Fujita on the previous version, I don't think that we
> need to add the `DriverVTable` to the `match_phy_device` function if we
> don't provide accessor methods. Currently that isn't needed, so you only
> need the hunks above this one. (I'd wait for Fujita's reply though).

Agreed, to make DriverVTable actually useful in match_phy_device(),
further changes would be needed. I think it's sufficient to simply
make the Rust code compile, as shown in the patch below.

I can take care of making sure Rust uses DriverVTable correctly later.

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index a59469c785e3..32ea43ece646 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -421,6 +421,7 @@ impl<T: Driver> Adapter<T> {
     /// `phydev` must be passed by the corresponding callback in `phy_driver`.
     unsafe extern "C" fn match_phy_device_callback(
         phydev: *mut bindings::phy_device,
+        _phydrv: *const bindings::phy_driver,
     ) -> crate::ffi::c_int {
         // SAFETY: This callback is called only in contexts
         // where we hold `phy_device->lock`, so the accessors on