Re: [PATCH v2 net] net: enetc: fix VSI mailbox timeout handling and DMA lifecycle

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: enetc: fix VSI mailbox timeout handling and DMA lifecycle

This patch preserves the VSI mailbox DMA buffer in si->msg so it can
be freed on the next send (once the mailbox is no longer busy), and
adds a final free in enetc_vf_remove(). It also bails out early if
the mailbox is still busy when a new message is submitted.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 6c4b374bcb0e..e8c5adee9743 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c

[ ... ]

> @@ -258,6 +281,7 @@ static int enetc_vf_probe(struct pci_dev *pdev,
>  static void enetc_vf_remove(struct pci_dev *pdev)
>  {
>  	struct enetc_si *si = pci_get_drvdata(pdev);
> +	struct enetc_msg_swbd msg = si->msg;
>  	struct enetc_ndev_priv *priv;
>  
>  	priv = netdev_priv(si->ndev);
>  	unregister_netdev(si->ndev);

Is there a race between this local copy of si->msg and a concurrent
ndo callback?

The local 'msg' is captured at function entry, before unregister_netdev()
has run, and .remove() does not hold RTNL. During that window, userspace
(for example 'ip link set dev ethX address ...') can acquire RTNL and
invoke enetc_vf_set_mac_addr() -> enetc_msg_vsi_set_primary_mac_addr()
-> enetc_msg_vsi_send() on the same si.

Inside enetc_msg_vsi_send(), on the non-busy path:

	/* Free the DMA buffer of the last message */
	enetc_msg_dma_free(dev, &si->msg);
	si->msg = *msg;

the buffer whose (vaddr, dma, size) our local 'msg' still holds a copy
of gets freed, and si->msg is replaced with a new allocation.

>  
>  	enetc_free_msix(priv);
>  
>  	enetc_free_si_resources(priv);
>  	enetc_teardown_cbdr(&si->cbd_ring);
>  
>  	free_netdev(si->ndev);
>  
>  	enetc_pci_remove(pdev);
> +	enetc_msg_dma_free(&pdev->dev, &msg);
>  }

When this trailing enetc_msg_dma_free(&pdev->dev, &msg) runs, can it
call dma_free_coherent() on a buffer that enetc_msg_vsi_send() already
freed, and also leak the newer buffer now sitting in si->msg (which is
no longer referenced by anything)?

Would capturing msg = si->msg after unregister_netdev() returns (i.e.
once no further ndo callbacks can race) avoid both the double free
and the leak?
-- 
pw-bot: cr