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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Sun, May 3, 2026 10:16 AM, Jakub Kicinski wrote:
> 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?

For the hardware design on this device, WOL packets are handled before DMA ring.
So programming a zeroed ring->dma does not result in DMA transactions to
physical address 0. At this point, the original code is fine.

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