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

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

[Bobby Eshleman]

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().  That
function checks fbn->pcs and, finding it non-NULL, calls
xpcs_destroy_pcs() a second time on the already-freed object, triggering
a refcount underflow use-after-free.

[   1.934973] fbnic 0000:01:00.0: Failed to create Phylink interface, err: -22
[   1.935103] ------------[ cut here ]------------
[   1.935179] refcount_t: underflow; use-after-free.
[   1.935252] WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90, CPU#0: swapper/0/1
[   1.935389] Modules linked in:
[   1.935484] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.0.0-virtme-04244-g1f5ffc672165-dirty #1 PREEMPT(lazy)
[   1.935661] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[   1.935826] RIP: 0010:refcount_warn_saturate+0x59/0x90
[   1.935931] Code: 44 48 8d 3d 49 f9 a7 01 67 48 0f b9 3a e9 bf 1e 96 00 48 8d 3d 48 f9 a7 01 67 48 0f b9 3a c3 cc cc cc cc 48 8d 3d 47 f9 a7 01 <67> 48 0f b9 3a c3 cc cc cc cc 48 8d 3d 46 f9 a7 01 67 48 0f b9 3a
[   1.936274] RSP: 0000:ffffd0d440013c58 EFLAGS: 00010246
[   1.936376] RAX: 0000000000000000 RBX: ffff8f39c188c278 RCX: 000000000000002b
[   1.936524] RDX: ffff8f39c004f000 RSI: 0000000000000003 RDI: ffffffff96abab00
[   1.936692] RBP: ffff8f39c188c240 R08: ffffffff96988e88 R09: 00000000ffffdfff
[   1.936835] R10: ffffffff96878ea0 R11: 0000000000000187 R12: 0000000000000000
[   1.936970] R13: ffff8f39c0cef0c8 R14: ffff8f39c1ac01c0 R15: 0000000000000000
[   1.937114] FS:  0000000000000000(0000) GS:ffff8f3ba08b4000(0000) knlGS:0000000000000000
[   1.937273] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   1.937382] CR2: ffff8f3b3ffff000 CR3: 0000000172642001 CR4: 0000000000372ef0
[   1.937540] Call Trace:
[   1.937619]  <TASK>
[   1.937698]  xpcs_destroy_pcs+0x25/0x40
[   1.937783]  fbnic_netdev_alloc+0x1e5/0x200
[   1.937859]  fbnic_probe+0x230/0x370
[   1.937939]  local_pci_probe+0x3e/0x90
[   1.938013]  pci_device_probe+0xbb/0x1e0
[   1.938091]  ? sysfs_do_create_link_sd+0x6d/0xe0
[   1.938188]  really_probe+0xc1/0x2b0
[   1.938282]  __driver_probe_device+0x73/0x120
[   1.938371]  driver_probe_device+0x1e/0xe0
[   1.938466]  __driver_attach+0x8d/0x190
[   1.938560]  ? __pfx___driver_attach+0x10/0x10
[   1.938663]  bus_for_each_dev+0x7b/0xd0
[   1.938758]  bus_add_driver+0xe8/0x210
[   1.938854]  driver_register+0x60/0x120
[   1.938929]  ? __pfx_fbnic_init_module+0x10/0x10
[   1.939026]  fbnic_init_module+0x25/0x60
[   1.939109]  do_one_initcall+0x49/0x220
[   1.939202]  ? rdinit_setup+0x20/0x40
[   1.939304]  kernel_init_freeable+0x1b0/0x310
[   1.939449]  ? __pfx_kernel_init+0x10/0x10
[   1.939560]  kernel_init+0x1a/0x1c0
[   1.939640]  ret_from_fork+0x1ed/0x240
[   1.939730]  ? __pfx_kernel_init+0x10/0x10
[   1.939805]  ret_from_fork_asm+0x1a/0x30
[   1.939886]  </TASK>
[   1.939927] ---[ end trace 0000000000000000 ]---
[   1.940184] fbnic 0000:01:00.0: Netdev allocation failed

Fix by clearing fbn->pcs immediately after the error-path destroy so
fbnic_phylink_destroy() skips the second call.

Fixes: d0fe7104c795 ("fbnic: Replace use of internal PCS w/ Designware XPCS")
Assisted-by: Claude:claude-4.5-sonnet
Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_phylink.c | 1 +
 1 file changed, 1 insertion(+)

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;
 	}
 

---
base-commit: ccd8e87748ad083047d6c8544c5809b7f96cc8df
change-id: 20260416-fbnic-pcs-fix-26dc23c7deae

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

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

[Andrew Lunn]

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

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

[Bobby Eshleman]

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

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

[Andrew Lunn]

> 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;

Why set it to NULL? Setting pointers to NULL like this often suggests
you are not confident the code is correct and you are being
defensive. It is better to review the code and be sure it does the
correct thing.

>  		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;

Why set it to NULL? If it failed, you are unwinding and about to fail
the probe. Nothing should be using it.

I would also say fbnic_phylink_destroy() is wrong or at least whoever
wrote it is not confident in there own code. It should only be called
if fbnic_phylink_create() was successful, so you know fbn->pcs is
valid, so there is no need to test it. The same for fbn->phylink.

       Andrew

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

[Bobby Eshleman]

On Fri, Apr 17, 2026 at 01:48:38AM +0200, Andrew Lunn wrote:
> > 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;
> 
> Why set it to NULL? Setting pointers to NULL like this often suggests
> you are not confident the code is correct and you are being
> defensive. It is better to review the code and be sure it does the
> correct thing.
> 
> >  		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;
> 
> Why set it to NULL? If it failed, you are unwinding and about to fail
> the probe. Nothing should be using it.
> 
> I would also say fbnic_phylink_destroy() is wrong or at least whoever
> wrote it is not confident in there own code. It should only be called
> if fbnic_phylink_create() was successful, so you know fbn->pcs is
> valid, so there is no need to test it. The same for fbn->phylink.
> 
>        Andrew

Fair points. I think it looks sound without resetting to NULL, but I'll
double check and remove if confident.

I'll look at the checks in _destroy() too.

Thanks,
Bobby