Re: [PATCH net-next v2 5/6] net: wangxun: clear stored DMA addresses after dma_free_coherent()

[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: wangxun: clear stored DMA addresses after dma_free_coherent()

This commit clears the ring DMA addresses after freeing their resources via
dma_free_coherent() in the wangxun driver. It aims to prevent use-after-free
of the DMA rings at shutdown when WOL is enabled.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 3216dee778be7..51599f6b878e7 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -2462,6 +2462,7 @@ void wx_free_isb_resources(struct wx *wx)
>  	dma_free_coherent(&pdev->dev, sizeof(u32) * 4,
>  			  wx->isb_mem, wx->isb_dma);
>  	wx->isb_mem = NULL;
> +	wx->isb_dma = 0;
>  }

This isn't a regression introduced by this patch, but can dma_free_coherent()
be called with a NULL wx->isb_mem virtual address here?

Since wx->isb_mem is only allocated when the interface is brought up, if the
module is removed while the interface is down, wx->isb_mem will be NULL. Does
passing a NULL virtual address to dma_free_coherent() cause a panic here, and
should it be guarded by a NULL check?

[ ... ]

> @@ -2678,6 +2679,7 @@ static void wx_free_rx_resources(struct wx_ring *rx_ring)
>  			  rx_ring->desc, rx_ring->dma);
>  
>  	rx_ring->desc = NULL;
> +	rx_ring->dma = 0;

If rx_ring->dma is set to 0 here, and Wake-on-LAN is enabled, does this result
in the hardware being programmed to DMA directly to physical address 0?

Looking at ngbe_dev_shutdown(), it frees these resources and then calls
wx_configure_rx(). Inside wx_configure_rx_ring(), the driver programs the
base address registers WX_PX_RR_BAL with ring->dma, which is now 0. When a
WOL packet arrives, could this cause silent memory corruption or fatal
IOMMU faults?

Additionally, this wasn't introduced by this patch, but does the shutdown
sequence trigger a NULL pointer dereference when WOL is active?

Earlier in wx_free_rx_resources(), rx_ring->rx_buffer_info is freed and set
to NULL, but ring->count retains its original value. During WOL shutdown,
ngbe_dev_shutdown() frees these resources and then reconfigures the RX rings
via wx_configure_rx_ring(). 

If wx_configure_rx_ring() executes:
memset(ring->rx_buffer_info, 0, sizeof(struct wx_rx_buffer) * ring->count);

with a NULL pointer and a count > 0, won't this cause a kernel panic?

[ ... ]

> @@ -2906,6 +2910,7 @@ static int wx_setup_rx_resources(struct wx_ring *rx_ring)
>  
>  err_desc:
>  	dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> +	rx_ring->dma = 0;
>  err:
>  	kvfree(rx_ring->rx_buffer_info);
>  	rx_ring->rx_buffer_info = NULL;

Should rx_ring->desc also be set to NULL here?

If wx_alloc_page_pool() fails, the driver jumps to err_desc and frees the DMA
memory, but leaves rx_ring->desc pointing to the freed memory. Since wx_ring
structures can be reused across interface up/down cycles, could this dangling
pointer bypass the if (!rx_ring->desc) safety check in wx_free_rx_resources()
and cause a double-free later?