Re: [PATCH net] eth: fbnic: fix double-free of PCS on phylink creation failure

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Thu, Apr 16, 2026 at 11:00:01PM +0200, Andrew Lunn wrote:
> On Thu, Apr 16, 2026 at 12:31:39PM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> > 
> > fbnic_phylink_create() stores the newly allocated PCS in fbn->pcs before
> > calling phylink_create(). When phylink_create() fails the error path
> > calls xpcs_destroy_pcs(pcs) to release the PCS, but neglects to clear
> > fbn->pcs.
> > 
> > The caller, fbnic_netdev_alloc(), responds to the failure by calling
> > fbnic_netdev_free() which in turn calls fbnic_phylink_destroy().
> 
> Isn't the real problem here? fbnic_phylink_create() failed, and it
> correctly did its cleanup. Because it failed, you should not be
> calling fbnic_phylink_destroy(). You only call the _destroy() if the
> previous _create() was successful.
> 
>     Andrew
> 
> ---
> pw-bot: cr

True, the real issue is the _create/_destroy call asymmetry.

How about something like this (untested)?:

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index e3ca5fcfabef..2a6a73393732 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -818,7 +818,8 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd)
 	netif_tx_stop_all_queues(netdev);
 
 	if (fbnic_phylink_create(netdev)) {
-		fbnic_netdev_free(fbd);
+		free_netdev(netdev);
+		fbd->netdev = NULL;
 		return NULL;
 	}
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
index 09c5225111be..50240e6c2ee9 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
@@ -237,6 +237,7 @@ int fbnic_phylink_create(struct net_device *netdev)
 		dev_err(netdev->dev.parent,
 			"Failed to create Phylink interface, err: %d\n", err);
 		xpcs_destroy_pcs(pcs);
+		fbn->pcs = NULL;
 		return err;
 	}
 


I think we still probably want to be NULL-ing the pcs, even if it won't
necessarily matter anymore.

Best,
Bobby