netdev - Re: [PATCH net-next v7 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]

Message-ID: <ZT6fzfV9GUQOZnlx@boqun-archlinux>
Date: Sun, 29 Oct 2023 11:09:17 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: benno.lossin@...ton.me, andrew@...n.ch, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, tmgross@...ch.edu,
	miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY
 drivers

On Sun, Oct 29, 2023 at 09:48:41AM -0700, Boqun Feng wrote:
> On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> [...]
> > 
> > The current code is fine from Rust perspective because the current
> > code copies phy_driver on stack and makes a reference to the copy, if
> > I undertand correctly.
> > 
> 
> I had the same thought Benno brought the issue on `&`, but unfortunately
> it's not true ;-) In the following code:
> 
> 	let phydev = unsafe { *self.0.get() };
> 
> , semantically the *whole* `bindings::phy_device` is being read, so if
> there is any modification (i.e. write) that may happen in the meanwhile,
> it's data race, and data races are UB (even in C).
> 
> So both implementations have the problem because of the same cause.
> 
> > It's not nice to create an 500-bytes object on stack. It turned out
> > that it's not so simple to avoid it.
> 
> As you can see, copying is not the way to work around this.
> 

An temporary solution is doing the #2 option from Benno, but in Rust and
open code it, like the following:

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 145d0407fe31..f5230ac48014 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -121,10 +121,10 @@ pub fn state(&self) -> DeviceState {
     ///
     /// It returns true if the link is up.
     pub fn is_link_up(&self) -> bool {
-        const LINK_IS_UP: u32 = 1;
+        const LINK_IS_UP: u64 = 1;
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.link() == LINK_IS_UP
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(14, 1) == LINK_IS_UP
     }
 
     /// Gets the current auto-negotiation configuration.
@@ -132,18 +132,18 @@ pub fn is_link_up(&self) -> bool {
     /// It returns true if auto-negotiation is enabled.
     pub fn is_autoneg_enabled(&self) -> bool {
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.autoneg() == bindings::AUTONEG_ENABLE
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
     pub fn is_autoneg_completed(&self) -> bool {
-        const AUTONEG_COMPLETED: u32 = 1;
+        const AUTONEG_COMPLETED: u64 = 1;
         // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        let phydev = unsafe { *self.0.get() };
-        phydev.autoneg_complete() == AUTONEG_COMPLETED
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(15, 1) == AUTONEG_COMPLETED
     }
 
     /// Sets the speed of the PHY.


Of course, it's not maintainable in longer term since it relies on
hard-coding the bit offset of these bit fields. But I think it's best we
can do from Linux kernel side. It's up to Andrew and Miguel whether this
temporary solution is OK.

Regards,
Boqun