Re: [net-next,v6,04/15] ibmveth: Refactor buffer pool management for per-queue MQ RX

[mingming cao <mmc@linux.ibm.com>]

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 (1) · Medium: 0 · Low: 2
>
> - [Low] The refactor silently repairs a pre-existing DMA-coherent memory
>    leak in ibmveth_open() without a Fixes: tag.
> - [Low] The err_cleanup comment in ibmveth_alloc_rx_queues()
>    (drivers/net/ethernet/ibm/ibmveth.c:280-285) asserts an invariant the…
>
> Pre-existing issues:
> - [High] In ibmveth_open() at this commit, the new failure branch for
>    ibmveth_alloc_buffer_pools() (`goto out_free_tx_ltb`,…
>
> --
>
> ibmveth: Refactor buffer pool management for per-queue MQ RX
>
> This moves the five RX buffer pools from adapter scope to
> rx_buff_pool[queue][pool] and adds ibmveth_alloc_buffer_pools() /
> ibmveth_free_buffer_pools() plus per-queue helpers, wiring them into
> open() and close(). Queue 0 stays the template for pool geometry and
> activation; runtime is still single-queue.
>
> A few questions below.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index d2dad9e6068e0..37a6d13e603ea 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... helper additions snipped ... ]
>
>> @@ -858,16 +996,9 @@ static int ibmveth_open(struct net_device *netdev)
>>   		goto out_free_tx_ltb;
>>   	}
>>   
>> -	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> -		if (!adapter->rx_buff_pool[0][i].active)
>> -			continue;
>> -		if (ibmveth_alloc_buffer_pool(&adapter->rx_buff_pool[0][i])) {
>> -			netdev_err(netdev, "unable to alloc pool\n");
>> -			adapter->rx_buff_pool[0][i].active = 0;
>> -			rc = -ENOMEM;
>> -			goto out_free_buffer_pools;
>> -		}
>> -	}
>> +	rc = ibmveth_alloc_buffer_pools(adapter);
>> +	if (rc)
>> +		goto out_free_tx_ltb;
> [Severity: High]
>
> This isn't a bug introduced by this patch, but the rewritten jump lands on
> an unwind that frees memory PHYP was just told about, without any
> deregistration hypercall.
>
> At this point ibmveth_register_logical_lan() has already returned
> H_SUCCESS. The chosen label then does:
>
> out_free_tx_ltb:
> 	while (--i >= 0)
> 		ibmveth_free_tx_ltb(adapter, i);
> 	ibmveth_cleanup_rx_resources(adapter);
> out_free_filter_list:
> 	ibmveth_free_filter_list(adapter);
>
> and ibmveth_cleanup_rx_resources() dma_unmap_single()s buffer_list_dma[0],
> free_page()s buffer_list_addr[0] and dma_free_coherent()s
> rx_queue[0].queue_addr, while ibmveth_free_filter_list() releases the
> filter page. Those are exactly the three regions handed to the hypervisor
> by the registration.
>
> The neighbouring request_irq() failure path does the opposite order:
>
> 		do {
> 			lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
> 		} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>
> 		goto out_free_buffer_pools;
>
> Can PHYP write into the buffer-list page after it has gone back to the page
> allocator on the pool-failure path? It stays an independent writer of that
> page for the per-page no-buffer drop count that
> ibmveth_update_rx_no_buffer() reads back.
Yes, PHYP can write that page: the LAN is registered,
no buffers are posted, and this goto frees the buffer
list without h_free. Pre-existing — the old inline pool
loop already jumped to the same labels.

> The same ordering exists in the parent commit, and the rest of this series
> does address it: the later patch that refactors TX resource allocation
> issues h_free_logical_lan() before the RX DMA teardown, and the patch
> adding the RX queue register helpers moves ibmveth_alloc_buffer_pools()
> ahead of registration so this window disappears. Would it be worth
> ordering the pool allocation before registration here instead, so the
> intermediate commits in the series do not carry the window?
The pool-before-register reorder stays out of this
helper extract. Patch 6 ("Refactor TX resource
allocation in open/close paths") issues h_free on that
pool-fail path. Patch 7 ("Add RX queue register helpers
for MQ") then allocates the pools before register, and
every later failure goes through
ibmveth_free_all_queues().
>>   
>>   	netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
>>   	rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
>>   			 netdev);
>> @@ -892,11 +1023,7 @@ static int ibmveth_open(struct net_device *netdev)
>>   	return 0;
>>   
>>   out_free_buffer_pools:
>> -	while (--i >= 0) {
>> -		if (adapter->rx_buff_pool[0][i].active)
>> -			ibmveth_free_buffer_pool(adapter,
>> -						 &adapter->rx_buff_pool[0][i]);
>> -	}
>> +	ibmveth_free_buffer_pools(adapter);
>>   out_free_tx_ltb:
>>   	while (--i >= 0)
>>   		ibmveth_free_tx_ltb(adapter, i);
> [Severity: Low]
>
> The commit message describes the repair this hunk performs and then
> declines a tag:
>
>      Pulling the pool loop out has one side effect worth naming: it no
>      longer consumes open()'s loop index, so a pool failure reaches
>      out_free_tx_ltb with i still at real_num_tx_queues and the TX LTBs
>      actually get freed. The shared index that swallowed them was
>      pre-existing, so there is no standalone Fixes: tag;
>
> Should this carry a Fixes: tag for the commit that added out_free_tx_ltb
> and its second walk over the same index?
> Before this patch the old loop at out_free_buffer_pools drove i down to -1,
> so the following while (--i >= 0) at out_free_tx_ltb evaluated -2 and freed
> nothing. Every failed ndo_open after a successful TX LTB allocation
> therefore leaked real_num_tx_queues buffers, one dma_alloc_coherent() of
> adapter->tx_ltb_size each, reachable both from an
> ibmveth_alloc_buffer_pool() failure and from a request_irq() failure.
>
> Without a tag, stable trees keep the leak and the repair is only reachable
> by picking up this refactor.
The helper no longer consuming i is a real side
effect, but a Fixes: off this refactor would not
backport cleanly, and Patch 6 gives TX its own
unwind. That is why the commit message names the
side effect and declines the tag, and I am keeping
that in this 15. After this series lands, a small
standalone for net can carry the Fixes: tag so
stable can take the leak fix without this refactor.
>
> One further note on code this patch does not touch, but which the
> immediately preceding patch in the series added:
>
> [Severity: Low]
>
> Does the err_cleanup comment in ibmveth_alloc_rx_queues() state an
> invariant the function actually holds?
>
> 	/*
> 	 * Every failure path above releases what it had already allocated
> 	 * for queue i, so each index here is either fully constructed or
> 	 * fully empty. Do not unmap buffer_list_dma[] without the matching
> 	 * buffer_list_addr[] check: the two are only ever set together.
> 	 */
>
> The buffer-list mapping failure path leaves rx_queue[i].queue_addr
> allocated:
>
> 		if (dma_mapping_error(dev, adapter->buffer_list_dma[i])) {
> 			...
> 			free_page((unsigned long)adapter->buffer_list_addr[i]);
> 			adapter->buffer_list_addr[i] = NULL;
> 			adapter->buffer_list_dma[i] = 0;
> 			goto err_cleanup;
> 		}
>
> so index i arrives at err_cleanup partially constructed, and it is the
> per-pointer if (adapter->rx_queue[i].queue_addr) check that frees the ring.
> Nothing leaks today, but the first half of the comment contradicts the
> second half. Could the wording be changed to say the cleanup loop frees by
> pointer presence rather than claiming each index is all-or-nothing?
Yes — thanks. v7 already rewords that comment in Patch 3
("Refactor RX resource allocation for MQ RX bring-up").
Cleanup frees by pointer presence. A dma_mapping_error on
queue i can leave queue_addr live after buffer_list_addr is
already NULL. This patch does not touch that helper.

Thanks,
Mingming