[PATCH net v2] eth: fbnic: move aui and fec from fbnic_net to fbnic_dev

[PATCH net v2] eth: fbnic: move aui and fec from fbnic_net to fbnic_dev

[Bobby Eshleman]

From: Bobby Eshleman <bobbyeshleman@meta.com>

fbnic_mdio_read_pmd() reaches through fbd->netdev to netdev_priv() in
order to read fbn->aui:

	if (fbd->netdev) {
		fbn = netdev_priv(fbd->netdev);
		if (fbn->aui < FBNIC_AUI_UNKNOWN)
			aui = fbn->aui;
	}

This can lead to a TOCTOU bug where fbd->netdev is checked after being
freed but before being cleared.

Both aui and fec describe the link configuration of the NIC, so move
them to fbnic_dev together. The MDIO layer (and other fbd-level code)
can read them directly without bouncing through fbd->netdev. The
hazardous accesses through fbd->netdev are dropped.

Fixes: d0ce9fd7eae0 ("fbnic: Add SW shim for MDIO interface to PMD and PCS")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
---
Changes in v2:
- rebase over net/main
- Link to v1: https://lore.kernel.org/r/20260521-fbnic-aui-change-v1-1-fc0c46e8e682@meta.com
---
 drivers/net/ethernet/meta/fbnic/fbnic.h         |  3 +++
 drivers/net/ethernet/meta/fbnic/fbnic_irq.c     |  2 +-
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c     |  3 +--
 drivers/net/ethernet/meta/fbnic/fbnic_mdio.c    |  9 ++-------
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.h  |  3 ---
 drivers/net/ethernet/meta/fbnic/fbnic_phylink.c | 18 ++++++++++--------
 6 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index d0715695c43e..76cfaf77b5ef 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -88,6 +88,9 @@ struct fbnic_dev {
 	unsigned long end_of_pmd_training;
 	u8 pmd_state;
 
+	u8 aui;
+	u8 fec;
+
 	/* Local copy of hardware statistics */
 	struct fbnic_hw_stats hw_stats;
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_irq.c b/drivers/net/ethernet/meta/fbnic/fbnic_irq.c
index 5e383d40abc7..03dbb53b33d9 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_irq.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_irq.c
@@ -132,7 +132,7 @@ static irqreturn_t fbnic_mac_msix_intr(int __always_unused irq, void *data)
 	fbn = netdev_priv(fbd->netdev);
 
 	/* Record link down events */
-	if (!fbd->mac->get_link(fbd, fbn->aui, fbn->fec))
+	if (!fbd->mac->get_link(fbd, fbd->aui, fbd->fec))
 		phylink_pcs_change(fbn->pcs, false);
 
 	return IRQ_HANDLED;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
index 53b7a938b4c2..e911dbf41b46 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
@@ -515,7 +515,6 @@ static u32 __fbnic_mac_cmd_config_asic(struct fbnic_dev *fbd,
 	/* Enable MAC Promiscuous mode and Tx padding */
 	u32 command_config = FBNIC_MAC_COMMAND_CONFIG_TX_PAD_EN |
 			     FBNIC_MAC_COMMAND_CONFIG_PROMISC_EN;
-	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
 
 	/* Disable pause frames if not enabled */
 	if (!tx_pause)
@@ -524,7 +523,7 @@ static u32 __fbnic_mac_cmd_config_asic(struct fbnic_dev *fbd,
 		command_config |= FBNIC_MAC_COMMAND_CONFIG_RX_PAUSE_DIS;
 
 	/* Disable fault handling if no FEC is requested */
-	if (fbn->fec == FBNIC_FEC_OFF)
+	if (fbd->fec == FBNIC_FEC_OFF)
 		command_config |= FBNIC_MAC_COMMAND_CONFIG_FLT_HDL_DIS;
 
 	return command_config;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
index 709041f7fc43..49895430c97d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
@@ -5,7 +5,6 @@
 #include <linux/pcs/pcs-xpcs.h>
 
 #include "fbnic.h"
-#include "fbnic_netdev.h"
 
 #define DW_VENDOR		BIT(15)
 #define FBNIC_PCS_VENDOR	BIT(9)
@@ -15,18 +14,14 @@ static int
 fbnic_mdio_read_pmd(struct fbnic_dev *fbd, int addr, int regnum)
 {
 	u8 aui = FBNIC_AUI_UNKNOWN;
-	struct fbnic_net *fbn;
 	int ret = 0;
 
 	/* We don't need a second PMD, just one can handle both lanes */
 	if (addr)
 		return 0;
 
-	if (fbd->netdev) {
-		fbn = netdev_priv(fbd->netdev);
-		if (fbn->aui < FBNIC_AUI_UNKNOWN)
-			aui = fbn->aui;
-	}
+	if (fbd->aui < FBNIC_AUI_UNKNOWN)
+		aui = fbd->aui;
 
 	switch (regnum) {
 	case MDIO_DEVID1:
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
index eded20b0e9e4..213253a818bb 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
@@ -46,9 +46,6 @@ struct fbnic_net {
 	struct phylink_config phylink_config;
 	struct phylink_pcs *pcs;
 
-	u8 aui;
-	u8 fec;
-
 	/* Cached top bits of the HW time counter for 40b -> 64b conversion */
 	u32 time_high;
 	/* Protect readers of @time_offset, writers take @time_lock. */
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
index 09c5225111be..61463bcb6431 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
@@ -65,6 +65,7 @@ int fbnic_phylink_ethtool_ksettings_get(struct net_device *netdev,
 					struct ethtool_link_ksettings *cmd)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
+	struct fbnic_dev *fbd = fbn->fbd;
 	int err;
 
 	err = phylink_ethtool_ksettings_get(fbn->phylink, cmd);
@@ -72,7 +73,7 @@ int fbnic_phylink_ethtool_ksettings_get(struct net_device *netdev,
 		unsigned long *supp = cmd->link_modes.supported;
 
 		cmd->base.port = PORT_DA;
-		cmd->lanes = (fbn->aui & FBNIC_AUI_MODE_R2) ? 2 : 1;
+		cmd->lanes = (fbd->aui & FBNIC_AUI_MODE_R2) ? 2 : 1;
 
 		fbnic_phylink_get_supported_fec_modes(supp);
 	}
@@ -84,11 +85,12 @@ int fbnic_phylink_get_fecparam(struct net_device *netdev,
 			       struct ethtool_fecparam *fecparam)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
+	struct fbnic_dev *fbd = fbn->fbd;
 
-	if (fbn->fec & FBNIC_FEC_RS) {
+	if (fbd->fec & FBNIC_FEC_RS) {
 		fecparam->active_fec = ETHTOOL_FEC_RS;
 		fecparam->fec = ETHTOOL_FEC_RS;
-	} else if (fbn->fec & FBNIC_FEC_BASER) {
+	} else if (fbd->fec & FBNIC_FEC_BASER) {
 		fecparam->active_fec = ETHTOOL_FEC_BASER;
 		fecparam->fec = ETHTOOL_FEC_BASER;
 	} else {
@@ -96,7 +98,7 @@ int fbnic_phylink_get_fecparam(struct net_device *netdev,
 		fecparam->fec = ETHTOOL_FEC_OFF;
 	}
 
-	if (fbn->aui & FBNIC_AUI_MODE_PAM4)
+	if (fbd->aui & FBNIC_AUI_MODE_PAM4)
 		fecparam->fec |= ETHTOOL_FEC_AUTO;
 
 	return 0;
@@ -120,7 +122,7 @@ fbnic_phylink_mac_prepare(struct phylink_config *config, unsigned int mode,
 	struct fbnic_net *fbn = netdev_priv(netdev);
 	struct fbnic_dev *fbd = fbn->fbd;
 
-	fbd->mac->prepare(fbd, fbn->aui, fbn->fec);
+	fbd->mac->prepare(fbd, fbd->aui, fbd->fec);
 
 	return 0;
 }
@@ -140,7 +142,7 @@ fbnic_phylink_mac_finish(struct phylink_config *config, unsigned int mode,
 	struct fbnic_dev *fbd = fbn->fbd;
 
 	/* Retest the link state and restart interrupts */
-	fbd->mac->get_link(fbd, fbn->aui, fbn->fec);
+	fbd->mac->get_link(fbd, fbd->aui, fbd->fec);
 
 	return 0;
 }
@@ -227,10 +229,10 @@ int fbnic_phylink_create(struct net_device *netdev)
 	__set_bit(PHY_INTERFACE_MODE_25GBASER,
 		  fbn->phylink_config.supported_interfaces);
 
-	fbnic_mac_get_fw_settings(fbd, &fbn->aui, &fbn->fec);
+	fbnic_mac_get_fw_settings(fbd, &fbd->aui, &fbd->fec);
 
 	phylink = phylink_create(&fbn->phylink_config, NULL,
-				 fbnic_phylink_select_interface(fbn->aui),
+				 fbnic_phylink_select_interface(fbd->aui),
 				 &fbnic_phylink_mac_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);

---
base-commit: c66d7c3c1f173cf73a2a2f8302666d86beafff22
change-id: 20260513-fbnic-aui-change-42ac92da3d74

Best regards,
-- 
Bobby Eshleman <bobbyeshleman@meta.com>

Re: [PATCH net v2] eth: fbnic: move aui and fec from fbnic_net to fbnic_dev

[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.
---
eth: fbnic: move aui and fec from fbnic_net to fbnic_dev

This patch moves the aui and fec fields from struct fbnic_net to
struct fbnic_dev. It aims to prevent a time-of-check to time-of-use bug by
allowing the MDIO layer to read these fields directly from the device
structure instead of going through a potentially freed netdev.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
> index 709041f7fc43d..49895430c97d9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
[ ... ]
> @@ -15,18 +14,14 @@ static int
>  fbnic_mdio_read_pmd(struct fbnic_dev *fbd, int addr, int regnum)
>  {
>  	u8 aui = FBNIC_AUI_UNKNOWN;
> -	struct fbnic_net *fbn;
>  	int ret = 0;
>  
>  	/* We don't need a second PMD, just one can handle both lanes */
>  	if (addr)
>  		return 0;
>  
> -	if (fbd->netdev) {
> -		fbn = netdev_priv(fbd->netdev);
> -		if (fbn->aui < FBNIC_AUI_UNKNOWN)
> -			aui = fbn->aui;
> -	}
> +	if (fbd->aui < FBNIC_AUI_UNKNOWN)
> +		aui = fbd->aui;

[Severity: High]
This isn't a bug introduced by this patch, but does moving aui and fec to
struct fbnic_dev actually prevent a use-after-free here?

If fbnic_mdiobus_create() registers the MDIO bus using
devm_mdiobus_register(), devres-managed resources are cleaned up after the
driver's remove callback returns.

If fbnic_remove() frees the fbd structure, the bus remains fully active
between the time the remove function finishes and the devres cleanup
unregisters the MDIO bus.

Can a concurrent sysfs access invoke MDIO read callbacks which pass fbd
to this function, leading to a dereference of the freed fbd pointer when
reading fbd->aui?
-- 
pw-bot: cr

Re: [PATCH net v2] eth: fbnic: move aui and fec from fbnic_net to fbnic_dev

[Jakub Kicinski]

On Thu, 28 May 2026 17:37:14 -0700 Jakub Kicinski wrote:
> > @@ -15,18 +14,14 @@ static int
> >  fbnic_mdio_read_pmd(struct fbnic_dev *fbd, int addr, int regnum)
> >  {
> >  	u8 aui = FBNIC_AUI_UNKNOWN;
> > -	struct fbnic_net *fbn;
> >  	int ret = 0;
> >  
> >  	/* We don't need a second PMD, just one can handle both lanes */
> >  	if (addr)
> >  		return 0;
> >  
> > -	if (fbd->netdev) {
> > -		fbn = netdev_priv(fbd->netdev);
> > -		if (fbn->aui < FBNIC_AUI_UNKNOWN)
> > -			aui = fbn->aui;
> > -	}
> > +	if (fbd->aui < FBNIC_AUI_UNKNOWN)
> > +		aui = fbd->aui;  
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does moving aui and fec to
> struct fbnic_dev actually prevent a use-after-free here?
> 
> If fbnic_mdiobus_create() registers the MDIO bus using
> devm_mdiobus_register(), devres-managed resources are cleaned up after the
> driver's remove callback returns.
> 
> If fbnic_remove() frees the fbd structure, the bus remains fully active
> between the time the remove function finishes and the devres cleanup
> unregisters the MDIO bus.
> 
> Can a concurrent sysfs access invoke MDIO read callbacks which pass fbd
> to this function, leading to a dereference of the freed fbd pointer when
> reading fbd->aui?

Sorry for broadening the scope here a little bit but I think we should
fix this (as well). In the same series, but probably separate patch.
Just don't use devm_, it creates about as many bugs as it prevents.