[PATCH net v3] rust: net: phy: fix off-by-one bit positions in device status accessors

[PATCH net v3] rust: net: phy: fix off-by-one bit positions in device status accessors

[Chunfeng Song]

The hand-written bitfield offsets in is_link_up(), is_autoneg_enabled()
and is_autoneg_completed() were correct when the abstraction was
merged: at that time autoneg, link, and autoneg_complete were at bits
13, 14, and 15 of struct phy_device's first bitfield unit. Commit
2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct
phy_device") later inserted is_genphy_driven just before autoneg,
shifting the three fields up by one, so the accessors now read:

  is_link_up()           reads bit 14 = autoneg
  is_autoneg_enabled()   reads bit 13 = is_genphy_driven
  is_autoneg_completed() reads bit 15 = link

The official ax88796b Rust driver uses all three accessors in its
link-change handling, so it inherits the bug. On genphy-driven devices
is_genphy_driven is always 1, which masks the broken
is_autoneg_enabled() check.

Hard-coded offsets will silently break again on the next layout
change, so use the bindgen-generated accessors (link(), autoneg(),
autoneg_complete()), which are always consistent with the C layout,
and drop the hand-written numbers together with the TODO comment that
marked them as a stopgap.

Found by a static equivalence audit (C2RustDrv, a C-to-Rust driver
migration tool) that compares hand-written bitfield offsets against
the bindgen layout of struct phy_device. Verified by building the
bindings and checking the generated accessors; no runtime testing was
possible without PHY hardware.

Fixes: 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct phy_device")
Cc: stable@vger.kernel.org
Signed-off-by: Chunfeng Song <springbreeze@stu.pku.edu.cn>
---
v2 -> v3:
  - Drop the links to previous versions from the commit message
    (Miguel Ojeda).
  - Keep the changelog here, after the separator, as is customary.

v1 -> v2:
  - Use the bindgen-generated accessors (link(), autoneg(),
    autoneg_complete()) instead of the hard-coded bit numbers, as
    suggested by Andrew Lunn, since the offsets drift whenever the
    layout changes.
  - Update the Fixes: tag to 2796ff1e3dca, the commit that inserted
    is_genphy_driven and shifted the fields (the offsets were correct
    when the abstraction was merged in v6.8).
  - Add the net tree prefix to the subject.

rust/kernel/net/phy.rs | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 956cda573ddb..14dd1e6cb380 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -124,38 +124,30 @@ pub fn state(&self) -> DeviceState {
     ///
     /// It returns true if the link is up.
     pub fn is_link_up(&self) -> bool {
-        const LINK_IS_UP: u64 = 1;
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
+        let phydev = self.0.get();
         // SAFETY: The struct invariant ensures that we may access
         // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(14, 1) == LINK_IS_UP
+        unsafe { (*phydev).link() == 1 }
     }
 
     /// Gets the current auto-negotiation configuration.
     ///
     /// It returns true if auto-negotiation is enabled.
     pub fn is_autoneg_enabled(&self) -> bool {
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
+        let phydev = self.0.get();
         // SAFETY: The struct invariant ensures that we may access
         // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(13, 1) == u64::from(bindings::AUTONEG_ENABLE)
+        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
     pub fn is_autoneg_completed(&self) -> bool {
-        const AUTONEG_COMPLETED: u64 = 1;
-        // TODO: the code to access to the bit field will be replaced with automatically
-        // generated code by bindgen when it becomes possible.
+        let phydev = self.0.get();
         // SAFETY: The struct invariant ensures that we may access
         // this field without additional synchronization.
-        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
-        bit_field.get(15, 1) == AUTONEG_COMPLETED
+        unsafe { (*phydev).autoneg_complete() == 1 }
     }
 
     /// Sets the speed of the PHY.
-- 
2.43.0

Re: [PATCH net v3] rust: net: phy: fix off-by-one bit positions in device status accessors

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
rust: net: phy: fix off-by-one bit positions in device status accessors

This replaces the hand-written bitfield offsets in is_link_up(),
is_autoneg_enabled() and is_autoneg_completed() with the bindgen-generated
link(), autoneg() and autoneg_complete() accessors, so the Rust side no
longer depends on hard-coded bit numbers for struct phy_device.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 956cda573ddba..14dd1e6cb3802 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -124,38 +124,30 @@ pub fn state(&self) -> DeviceState {
>      ///
>      /// It returns true if the link is up.
>      pub fn is_link_up(&self) -> bool {
> -        const LINK_IS_UP: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(14, 1) == LINK_IS_UP
> +        unsafe { (*phydev).link() == 1 }
>      }

[Severity: Medium]
Bindgen emits the bitfield getters with the signature

  pub fn link(&self) -> ::core::ffi::c_uint

so writing (*phydev).link() auto-refs the receiver and materializes a
&bindings::phy_device covering the whole C struct, not just the bitfield
storage unit that the previous code projected with
&(*self.0.get())._bitfield_1.

Is forming that whole-struct shared reference allowed here? The pointer
comes from an Opaque, whose documented contract in rust/kernel/types.rs
says:

  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on
  ///   the Rust side.

and the Opaque example reads fields through the raw pointer
(unsafe { (*ptr).val }) instead of taking a reference.

Concurrent C-side writes into the same allocation look possible while a
PHY driver callback runs, none of them covered by phydev->lock:

drivers/net/phy/phy.c:phy_interrupt() {
	...
		phydev->irq_rerun = 1;
		disable_irq_nosync(irq);
	...
}

That is a hard IRQ context write to a bitfield member without taking
phydev->lock. In addition, the embedded state_queue delayed_work is written
by the timer softirq and workqueue core, the embedded mdio.dev kref is
bumped by get_device()/put_device() in phy_device.c, and the internals of
phydev->lock itself (wait_lock, wait_list) are written by contending
threads.

Would reading the bits through a C __rust_helper accessor in rust/helpers/,
or projecting to the bitfield unit via a raw-pointer path, keep the goal of
dropping the hand-written offsets without forming a reference to the whole
struct?

Also, the retained SAFETY comments still say "we may access this field",
which no longer matches what the code does. Should they be updated to
describe the reference that is actually created?

>  
>      /// Gets the current auto-negotiation configuration.
>      ///
>      /// It returns true if auto-negotiation is enabled.
>      pub fn is_autoneg_enabled(&self) -> bool {
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(13, 1) == u64::from(bindings::AUTONEG_ENABLE)
> +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
>      }
>  
>      /// Gets the current auto-negotiation state.
>      ///
>      /// It returns true if auto-negotiation is completed.
>      pub fn is_autoneg_completed(&self) -> bool {
> -        const AUTONEG_COMPLETED: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(15, 1) == AUTONEG_COMPLETED
> +        unsafe { (*phydev).autoneg_complete() == 1 }
>      }

[Severity: Low]
This isn't a bug introduced by this patch, but after the rewrite
is_link_up(), is_autoneg_enabled() and is_autoneg_completed() are one-line
forwarders to a binding call, and none of them carries #[inline].
rust/kernel is an abstraction crate rather than a leaf driver, so the
coding guidelines ask for #[inline] on small functions that just forward to
a binding call.

The neighbouring accessors in the same file (phy_id(), state(),
set_speed(), set_duplex()) are equally unannotated, so this is the existing
style of the file rather than something the patch changes. Would it be
worth adding the annotation to the three touched functions here, or
handling all of them in a separate cleanup?
-- 
pw-bot: cr

Re: [PATCH net v3] rust: net: phy: fix off-by-one bit positions in device status accessors

[Chunfeng Song]

Thanks for the review.

You are right that the ordinary bindgen accessors take &self. Calling them
through (*phydev).link() creates a temporary shared reference to the
complete bindings::phy_device, which is not appropriate for the C-owned
object wrapped in Opaque.

For v4, I will use the bindgen-generated raw accessors link_raw(),
autoneg_raw(), and autoneg_complete_raw(), called as associated functions
with a raw pointer. They retain the bit positions and endianness handling
generated from the C layout without creating a Rust reference to the
complete phy_device.

I will also update the SAFETY comments to describe the validity of the raw
pointer and the callback-context preconditions. The raw accessors address
the reference-formation issue, but do not themselves provide locking,
READ_ONCE semantics, atomicity, or memory barriers.

I do not plan to add a C helper, since the generated raw accessors already
provide the required raw-pointer access. A plain C field helper would not
add synchronization semantics.

I will also add ordinary #[inline] annotations to the three small
forwarding methods touched by this patch.

The v4 patch will be posted as a separate thread after the required 24-hour
interval.