netdev - Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

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: <7edb5c43-f17b-4352-8c93-ae5bb9a54412@lunn.ch>
Date: Sat, 7 Oct 2023 17:13:26 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Trevor Gross <tmgross@...ch.edu>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
	greg@...ah.com
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

> The safety comment here still needs something like
> 
>     with the exception of fields that are synchronized via the `lock` mutex

I'm not sure that really adds much useful information. Which values
are protected by the lock? More importantly, which are not protected
by the lock?

As a general rule of thumb, driver writers don't understand
locking. Yes, there are some which do, but many don't. So the
workaround to that is make it so they don't need to understand
locking. All the locking happens in the core.

The exception is suspend and resume, which are called without the
lock. So if i was to add a comment about locking, i would only put a
comment on those two.

> > +    /// Executes software reset the PHY via BMCR_RESET bit.
> > +    pub fn genphy_soft_reset(&mut self) -> Result {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        // So an FFI call with a valid pointer.
> > +        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
> > +    }
> > +
> > +    /// Initializes the PHY.
> > +    pub fn init_hw(&mut self) -> Result {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        // so an FFI call with a valid pointer.
> > +        to_result(unsafe { bindings::phy_init_hw(phydev) })
> > +    }
> 
> Andrew, are there any restrictions about calling phy_init_hw more than
> once? Or are there certain things that you are not allowed to do until
> you call that function?

phy_init_hw can be called multiple times. It used by drivers as a work
around to broken hardware/firmware to get the device back into a good
state. It is also used during resume, since often the PHY looses its
settings when suspended.

> > +    unsafe extern "C" fn read_mmd_callback(
> > +        phydev: *mut bindings::phy_device,
> > +        devnum: i32,
> > +        regnum: u16,
> > +    ) -> i32 {
> > +        from_result(|| {
> > +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> > +            let dev = unsafe { Device::from_raw(phydev) };
> > +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> > +            Ok(ret.into())
> > +        })
> > +    }
> 
> Since your're reading a bus, it probably doesn't hurt to do a quick
> check when converting
> 
>     let devnum_u8 = u8::try_from(devnum).(|_| {
>         warn_once!("devnum {devnum} exceeds u8 limits");
>         code::EINVAL
>     })?

I would actually say this is the wrong place to do that. Such checks
should happen in the core, so it checks all drivers, not just the
current one Rust driver. Feel free to submit a C patch adding this.

	Andrew