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

[PATCH net v4] 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.

The ordinary bindgen accessors take &self. Calling them through
(*phydev).link() would create a shared reference to the complete
bindings::phy_device, which is not appropriate for an object wrapped in
Opaque.

Use the bindgen-generated raw accessors (link_raw(), autoneg_raw(),
and autoneg_complete_raw()) instead. They retain the bit positions and
endianness handling generated from the C layout without creating a Rust
reference to the complete phy_device. 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>
---
v4:
  - Use the bindgen-generated raw accessors to avoid creating a shared
    reference to the complete C `phy_device`, as suggested by Jakub Kicinski.
  - Update the SAFETY comments to describe pointer validity and the
    callback-context preconditions.
  - Add `#[inline]` to the three small forwarding methods.

v3: https://lore.kernel.org/r/20260825111334.795732-1-springbreeze@stu.pku.edu.cn/
  - Use the bindgen-generated accessors instead of hard-coded bit numbers.
  - Update the Fixes tag and add the net tree subject prefix.

v2: https://lore.kernel.org/r/20260824111227.742645-1-springbreeze@stu.pku.edu.cn/
  - Use bindgen-generated accessors as suggested by Andrew Lunn.
  - Explain that the offsets drifted after `is_genphy_driven` was added.

v1: https://lore.kernel.org/r/20260823103644.342849-1-springbreeze@stu.pku.edu.cn/
 rust/kernel/net/phy.rs | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 956cda573ddb..3f1c1637f027 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -123,39 +123,42 @@ pub fn state(&self) -> DeviceState {
     /// Gets the current link state.
     ///
     /// It returns true if the link is up.
+    #[inline]
     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.
-        // 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
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe { bindings::phy_device::link_raw(phydev) == 1 }
     }
 
     /// Gets the current auto-negotiation configuration.
     ///
     /// It returns true if auto-negotiation is enabled.
+    #[inline]
     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.
-        // 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)
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe {
+            bindings::phy_device::autoneg_raw(phydev) == bindings::AUTONEG_ENABLE
+        }
     }
 
     /// Gets the current auto-negotiation state.
     ///
     /// It returns true if auto-negotiation is completed.
+    #[inline]
     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.
-        // 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
+        let phydev = self.0.get().cast_const();
+        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
+        // `struct phy_device`, and the caller is in a context where this access
+        // is safe. The bindgen raw accessor does not create a Rust reference to
+        // the C struct.
+        unsafe { bindings::phy_device::autoneg_complete_raw(phydev) == 1 }
     }
 
     /// Sets the speed of the PHY.

base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
-- 
2.43.0

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

[Gary Guo]

On Tue Sep 1, 2026 at 4:18 AM BST, Chunfeng Song wrote:
> 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.
>
> The ordinary bindgen accessors take &self. Calling them through
> (*phydev).link() would create a shared reference to the complete
> bindings::phy_device, which is not appropriate for an object wrapped in
> Opaque.
>
> Use the bindgen-generated raw accessors (link_raw(), autoneg_raw(),
> and autoneg_complete_raw()) instead. They retain the bit positions and
> endianness handling generated from the C layout without creating a Rust
> reference to the complete phy_device. 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.

Might be worth mentioning that this is change is only possible since this
April's bindgen version bump.

This is a bindgen version that we requested
https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
bindgen 0.71.

This will affect backports.

>
> 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>
> ---
> v4:
>   - Use the bindgen-generated raw accessors to avoid creating a shared
>     reference to the complete C `phy_device`, as suggested by Jakub Kicinski.
>   - Update the SAFETY comments to describe pointer validity and the
>     callback-context preconditions.
>   - Add `#[inline]` to the three small forwarding methods.
>
> v3: https://lore.kernel.org/r/20260825111334.795732-1-springbreeze@stu.pku.edu.cn/
>   - Use the bindgen-generated accessors instead of hard-coded bit numbers.
>   - Update the Fixes tag and add the net tree subject prefix.
>
> v2: https://lore.kernel.org/r/20260824111227.742645-1-springbreeze@stu.pku.edu.cn/
>   - Use bindgen-generated accessors as suggested by Andrew Lunn.
>   - Explain that the offsets drifted after `is_genphy_driven` was added.
>
> v1: https://lore.kernel.org/r/20260823103644.342849-1-springbreeze@stu.pku.edu.cn/
>  rust/kernel/net/phy.rs | 43 ++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 956cda573ddb..3f1c1637f027 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -123,39 +123,42 @@ pub fn state(&self) -> DeviceState {
>      /// Gets the current link state.
>      ///
>      /// It returns true if the link is up.
> +    #[inline]
>      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.
> -        // 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
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe { bindings::phy_device::link_raw(phydev) == 1 }
>      }
>  
>      /// Gets the current auto-negotiation configuration.
>      ///
>      /// It returns true if auto-negotiation is enabled.
> +    #[inline]
>      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.
> -        // 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)
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe {
> +            bindings::phy_device::autoneg_raw(phydev) == bindings::AUTONEG_ENABLE

IMO it's little bit awkward to have the comparision be inside unsafe block.
Perhaps

    let autoneg = unsafe { bindings::phy_device::autoneg_raw(phydev) };
    autoneg == bindings::AUTONEG_ENABLE

?

Best,
Gary

> +        }
>      }
>  
>      /// Gets the current auto-negotiation state.
>      ///
>      /// It returns true if auto-negotiation is completed.
> +    #[inline]
>      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.
> -        // 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
> +        let phydev = self.0.get().cast_const();
> +        // SAFETY: By the type invariant of `Device`, `phydev` points to a valid
> +        // `struct phy_device`, and the caller is in a context where this access
> +        // is safe. The bindgen raw accessor does not create a Rust reference to
> +        // the C struct.
> +        unsafe { bindings::phy_device::autoneg_complete_raw(phydev) == 1 }
>      }
>  
>      /// Sets the speed of the PHY.
>
> base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6

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

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:03 PM Gary Guo <gary@garyguo.net> wrote:
>
> Might be worth mentioning that this is change is only possible since this
> April's bindgen version bump.
>
> This is a bindgen version that we requested
> https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
> bindgen 0.71.
>
> This will affect backports.

Definitely, if the patch shouldn't be automatically backported, then
we should limit it with a `#` comment, e.g.

  Cc: stable@vger.kernel.org # Only 7.1.y and later (requires
bindgen's raw pointer accessors).

> IMO it's little bit awkward to have the comparision be inside unsafe block.
> Perhaps
>
>     let autoneg = unsafe { bindings::phy_device::autoneg_raw(phydev) };
>     autoneg == bindings::AUTONEG_ENABLE
>
> ?

Agreed, yeah.

Cheers,
Miguel

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

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:47 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Definitely, if the patch shouldn't be automatically backported, then
> we should limit it with a `#` comment, e.g.
>
>   Cc: stable@vger.kernel.org # Only 7.1.y and later (requires
> bindgen's raw pointer accessors).

And then we should have a custom version sent to the other versions, of course.

Cheers,
Miguel

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

[Andrew Lunn]

> Might be worth mentioning that this is change is only possible since this
> April's bindgen version bump.
> 
> This is a bindgen version that we requested
> https://github.com/rust-lang/rust-bindgen/issues/2674 and available since
> bindgen 0.71.
> 
> This will affect backports.

Probably a FAQ, but what is the policy about toolchain versions for
stable kernels?

       Andrew

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

[Miguel Ojeda]

On Tue, Sep 1, 2026 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Probably a FAQ, but what is the policy about toolchain versions for
> stable kernels?

As far as I understand it, we are supposed to avoid changes unless
they are truly needed.

So there can be exceptions, but I doubt we can easily change a minimum
version without breaking a lot of users (e.g. distributions or users
using those distributions that have a particular version provided).

But Cc'ing stable just in case.

Cheers,
Miguel

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

[Greg KH]

On Tue, Sep 01, 2026 at 02:51:12PM +0200, Miguel Ojeda wrote:
> On Tue, Sep 1, 2026 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Probably a FAQ, but what is the policy about toolchain versions for
> > stable kernels?
> 
> As far as I understand it, we are supposed to avoid changes unless
> they are truly needed.
> 
> So there can be exceptions, but I doubt we can easily change a minimum
> version without breaking a lot of users (e.g. distributions or users
> using those distributions that have a particular version provided).
> 
> But Cc'ing stable just in case.

Stable kernels should be able to build with newer toolchains, but we
shouldn't break them by forcing a toolchain update unless there is a
very good reason to do so.

thanks,

greg k-h