Re: [PATCH v3 net 2/4] net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path

[Jijie Shao <shaojijie@huawei.com>]

on 2026/11/3 18:18, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit message says the new cleanup in
>    hns3_alloc_ring_memory()…
>
> --
>
> net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path
>
> When hns3_alloc_ring_buffers() fails for an RX ring, the out_with_desc
> error path in hns3_alloc_ring_memory() frees the descriptor ring. It does
> not destroy the page_pool that hns3_alloc_page_pool() just created. This
> patch adds a guarded page_pool_destroy() to that error path.
>
>> Add page_pool_destroy() to the out_with_desc error path, guarded by
>> a NULL check, mirroring the cleanup done in hns3_free_ring_memory().
> [Severity: Low]
> This isn't a bug, but is hns3_free_ring_memory() the right name here? I
> can't find a function with that name under
> drivers/net/ethernet/hisilicon/hns3/.
>
> The only other RX page_pool_destroy() is in hns3_fini_ring(). That function
> calls hns3_free_desc() first and destroys the pool last:
>
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_fini_ring() {
> 	hns3_free_desc(ring);
> 	devm_kfree(ring_to_dev(ring), ring->desc_cb);
>      ...
> 	if (!HNAE3_IS_TX_RING(ring) && ring->page_pool) {
> 		page_pool_destroy(ring->page_pool);
> 		ring->page_pool = NULL;
> 	}
> }
>
> The new out_with_desc path does it in the opposite order. It destroys the
> pool and sets ring->page_pool to NULL, then calls hns3_free_desc().
>
> That order looks harmless today. By the time hns3_alloc_ring_buffers()
> returns an error, it has already detached every buffer it attached. Each
> desc_cb[i].dma is zero, so hns3_free_buffer_detach() returns early and
> never hands a page back to the NULL pool.
>
> Could the commit message name hns3_fini_ring() instead? It could also drop
> the word "mirroring", since the order differs.

Agreed, that function does not exist. Will rename to
hns3_fini_ring() and drop "mirroring" in v4, since the
destroy order differs (pool-then-desc here vs desc-then-pool
in hns3_fini_ring()). The opposite order is safe in this
error path because hns3_alloc_ring_buffers() has already
detached every buffer.