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
| ||
|
Message-ID: <98471d44-c267-4c80-ba54-82ab2563e465@proton.me> Date: Tue, 17 Oct 2023 07:06:38 +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, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com Subject: Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY drivers On 15.10.23 00:39, FUJITA Tomonori wrote: > On Sat, 14 Oct 2023 17:07:09 +0000 > Benno Lossin <benno.lossin@...ton.me> wrote: >>> btw, methods in Device calling a C side function like mdiobus_read, >>> mdiobus_write, etc which never touch phydev->lock. Note that the c >>> side functions in resume()/suspned() methods don't touch phydev->lock >>> too. >>> >>> There are two types how the methods in Device changes the C side data. >>> >>> 1. read/write/read_paged >>> >>> They call the C side functions, mdiobus_read, mdiobus_write, >>> phy_read_paged, respectively. >>> >>> phy_device has a pointer to mii_bus object. It has stats for >>> read/write. So everytime they are called, stats is updated. >> >> I think for reading & updating some stats using `&self` >> should be fine. `write` should probably be `&mut self`. > > Can you tell me why exactly you think in that way? > > Firstly, you think that reading & updating some stats using `&self` should be fine. > > What's the difference between read() and set_speed(), which you think, needs &mut self. > > Because set_speed() updates the member in phy_device and read() > updates the object that phy_device points to? `set_speed` is entirely implemented on the Rust side and is not protected by a lock. Since data races in Rust are UB, this function must be `&mut`, in order to guarantee that no data races occur. This is the case, because our `Opaque` forces you to use interior mutability and thus sidestep this rule (modifying through a `&T`). > > > Secondly, What's the difference between read() and write(), where you > think that read() is &self write() is &mut self. This is just the standard Rust way of using mutability. For reading one uses `&self` and for writing `&mut self`. The only thing that is special here is the stats that are updated. But I thought that it still could fit Rust by the following pattern: ```rust pub struct TrackingReader { buf: [u8; 64], num_of_reads: Mutex<usize>, } impl TrackingReader { pub fn read(&self, idx: usize) -> u8 { *self.num_of_reads.lock() += 1; self.buf[idx] } } ``` And after taking a look at `mdiobus_read` I indeed found a mutex. > read() is reading from hardware register. write() is writing a value > to hardware register. Both updates the object that phy_device points > to? Indeed, I was just going with the standard way of suggesting `&self` for reads, there are of course exceptions where `&mut self` would make sense. That being said in this case both options are sound, since the C side locks a mutex. >>>> Since multiple `&self` references are allowed to coexist, you should >>>> use this for functions which perform their own serialization/do not >>>> require serialization. >>> >>> just to be sure, the C side guarantees that only one reference exists. >> >> I see, then the `from_raw` function should definitely return >> a `&mut Device`. Note that you can still call `&T` functions >> when you have a `&mut T`. > > It already returns &mut Device so no change is necessary here, right? Yes it already is correct. > unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self > { > > If you want more additional comment on from_raw(), please let me know. > > >>>> If you cannot decide what certain function receivers should be, then >>>> we can help you, but I would need more info on what the C side is doing. >>> >>> If you need more info on the C side, please let me know. >> >> What about these functions? >> - resolve_aneg_linkmode >> - genphy_soft_reset >> - init_hw >> - start_aneg >> - genphy_read_status >> - genphy_update_link >> - genphy_read_lpa >> - genphy_read_abilities > > As Andrew replied, all the functions update some member in phy_device. Do all of these functions lock the `bus->mdio_lock`? If yes, then you can just treat them like `read` or `write` (both `&self` and `&mut self` will be sound) and use the standard Rust way of setting the mutability. So if it changes some internal state, I would go with `&mut self`. -- Cheers, Benno
Powered by blists - more mailing lists