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