netdev - Re: [PATCH net-next v5 1/5] 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]

Date: Thu, 19 Oct 2023 13:57:39 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, andrew@...n.ch
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

On 19.10.23 02:41, FUJITA Tomonori wrote:
> On Wed, 18 Oct 2023 22:27:55 +0200
> Andrew Lunn <andrew@...n.ch> wrote:
> 
>>> +    /// Reads a given C22 PHY register.
>>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>>> +        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.
>>> +        let ret = unsafe {
>>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>
>> If i've understood the discussion about &mut, it is not needed here,
>> and for write. Performing a read/write does not change anything in
>> phydev. There was mention of statistics, but they are in the mii_bus
>> structure, which is pointed to by this structure, but is not part of
>> this structure.
> 
> If I understand correctly, he said that either (&self or &mut self) is
> fine for read().
> 
> https://lore.kernel.org/netdev/3469de1c-0e6f-4fe5-9d93-2542f87ffd0d@proton.me/
> 
> Since `&mut self` is unique, only one thread per instance of `Self`
> can call that function. So use this when the C side would use a lock.
> (or requires that only one thread calls that code)
> 
> Since multiple `&self` references are allowed to coexist, you should
> use this for functions which perform their own serialization/do not
> require serialization.
> 
> 
> I applied the first case here.

I will try to explain things a bit more.

So this case is a bit difficult to figure out, because what is
going on is not really a pattern that is used in Rust.
We already have exclusive access to the `phy_device`, so in Rust
you would not need to lock anything to also have exclusive access to the
embedded `mii_bus`. In this sense, mutable references (`&mut T`) are
infectious.

Since C always locks the `mdio_lock` when we call the read & write
functions, we however could also just use a shared reference (`&T`)
for the function receiver, since the C side guarantees serialization.

Another reason for choosing `&mut self` here is the following: it is
easier to later change to `&self` compared to going with `&self` now
and changing to `&mut self` later. This is because if you have a `&mut T`
you can also call all of its `&T` functions, but not the other way around.
`&mut self` is as a receiver also more conservative, since it is more
strict as to where it can be called. So let's just go with that.

-- 
Cheers,
Benno