Re: [PATCH net-next v4 05/15] gve: introduce control plane operations structure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Workqueue flushed after private resources are freed, leading to memory leaks and Use-After-Free.
--

--- Patch [5]: [PATCH net-next v4 05/15] gve: introduce control plane operations structure ---
commit 21d462ce52db670852717f6c821149b6dd730ea9
Author: Harshitha Ramamurthy <[email protected]>

gve: introduce control plane operations structure

To abstract out the difference in implementation of control plane operations
between the existing Adminq ABI and the upcoming Mailbox ABI, introduce a new
gve_ctrl_ops structure which will contain the basic operations.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -3000,14 +3005,13 @@ static void gve_remove(struct pci_dev *pdev)
>  {
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct gve_priv *priv = netdev_priv(netdev);
> -	__be32 __iomem *db_bar = priv->db_bar2;
>  	void __iomem *reg_bar = priv->reg_bar0;
>  
>  	unregister_netdev(netdev);
>  	gve_teardown_priv_resources(priv);
>  	destroy_workqueue(priv->gve_wq);
> +	priv->ctrl_ops->unmap_db_bar(priv);
>  	free_netdev(netdev);
> -	pci_iounmap(pdev, db_bar);
>  	pci_iounmap(pdev, reg_bar);
>  	pci_release_regions(pdev);
>  	pci_disable_device(pdev);

[Severity: High]
This is a pre-existing issue, but is it possible that tearing down private
resources before destroying the workqueue causes memory leaks and a use
after free?

If a service task is pending during gve_remove() and a reset was requested
via management IRQ, destroy_workqueue() will flush and execute it. 

The service task executes gve_reset() which can re-allocate resources and
re-register MSI-X interrupts.

Since gve_teardown_priv_resources() has already run, these newly allocated
resources won't be torn down before free_netdev() frees the priv pointer.

If the active MSI-X interrupt fires afterwards, would the interrupt handler
end up accessing the freed priv pointer?

It looks like this same ordering issue also exists in the probe error
handling path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5