Re: [PATCH net-next v5 03/15] ibmveth: Refactor RX resource allocation for MQ RX bring-up

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

On 8/17/26 6:47 PM, 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.
> ---
> ibmveth: Refactor RX resource allocation for MQ RX bring-up
>
> This moves the filter list and per-queue RX buffer list / RX ring
> allocation out of ibmveth_open() into four new helpers, and wires them
> into both the open failure unwind and ibmveth_close(). Runtime behaviour
> is meant to stay single-queue, and rc = -ENOMEM is now set before the TX
> LTB loop.
>
> A few questions below about the new release helpers and the remaining
> inline unwind in ibmveth_open().
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 7cb828b476c1b..168940bfa1094 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -151,6 +151,186 @@ static unsigned int ibmveth_real_max_tx_queues(void)
>>   	return min(n_cpu, IBMVETH_MAX_QUEUES);
>>   }
>>   
>> +/**
>> + * ibmveth_alloc_filter_list - Allocate and map filter list
>> + * @adapter: ibmveth adapter structure
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int
>> +ibmveth_alloc_filter_list(struct ibmveth_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +	struct net_device *netdev = adapter->netdev;
>> +
>> +	adapter->filter_list_addr = (void *)get_zeroed_page(GFP_KERNEL);
>> +	if (!adapter->filter_list_addr) {
>> +		netdev_err(netdev, "unable to allocate filter pages\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	adapter->filter_list_dma = dma_map_single(dev,
>> +						  adapter->filter_list_addr,
>> +						  4096, DMA_BIDIRECTIONAL);
>> +	if (dma_mapping_error(dev, adapter->filter_list_dma)) {
>> +		netdev_err(netdev, "unable to map filter list pages\n");
>> +		free_page((unsigned long)adapter->filter_list_addr);
>> +		adapter->filter_list_addr = NULL;
>> +		/* Do not leave DMA_MAPPING_ERROR for free_filter_list(). */
>> +		adapter->filter_list_dma = 0;
>> +		return -ENOMEM;
>> +	}
>> +
>> +	netdev_dbg(netdev, "filter list @ 0x%p (DMA: 0x%llx)\n",
>> +		   adapter->filter_list_addr,
>> +		   (unsigned long long)adapter->filter_list_dma);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ibmveth_free_filter_list - Free filter list resources
>> + * @adapter: ibmveth adapter structure
>> + */
>> +static void
>> +ibmveth_free_filter_list(struct ibmveth_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +
>> +	if (adapter->filter_list_dma) {
>> +		dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> +				 DMA_BIDIRECTIONAL);
>> +		adapter->filter_list_dma = 0;
>> +	}
>> +
>> +	if (adapter->filter_list_addr) {
>> +		free_page((unsigned long)adapter->filter_list_addr);
>> +		adapter->filter_list_addr = NULL;
>> +	}
>> +}
> [Severity: Medium]
> Is this patch doing more than a refactor?  The new pointer and DMA handle
> guards in ibmveth_free_filter_list(), plus the same guards in
> ibmveth_cleanup_rx_resources(), look like they also fix a double free that
> exists before this patch.
>
> In the baseline, the ibmveth_open() unwind released the pages without
> clearing anything:
>
> out_free_filter_list:
> 	free_page((unsigned long)adapter->filter_list_addr);
> out_free_buffer_list:
> 	free_page((unsigned long)adapter->buffer_list_addr[0]);
>
> and ibmveth_close() then repeated the same work unconditionally:
>
> 	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
> 			 DMA_BIDIRECTIONAL);
> 	free_page((unsigned long)adapter->buffer_list_addr[0]);
>
> 	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
> 			 DMA_BIDIRECTIONAL);
> 	free_page((unsigned long)adapter->filter_list_addr);
>
> ibmveth_set_csum_offload(), ibmveth_set_tso(), ibmveth_change_mtu() and
> veth_pool_store() all propagate an ibmveth_open() failure while leaving the
> interface running, so the following ndo_stop re-ran those frees on pages
> that were already returned, together with a dma_unmap of an already
> unmapped address.
>
> The applied commit message describes this as a pure refactor ("Runtime
> behavior stays single-queue"), and the reasoning for the guards only
> appears in the v5 changelog below the --- line, which is stripped when the
> patch is applied.  Could the commit message state that the double
> free_page() and stale dma_unmap are fixed here, and could a Fixes: tag be
> added so stable trees can pick it up?
>
> Note the sibling release helper ibmveth_free_tx_ltb() is still not
> idempotent at this point in the series; that is only addressed by the later
> "ibmveth: Refactor TX resource allocation in open/close paths" patch.

Yes. More than a refactor, and v6 names the double free_page() and
stale dma_unmap in the commit message rather than only below the ---.

The tag would be:

   Fixes: d43732ce021f ("ibmveth: properly unwind on init errors")

I would rather send it to net as a standalone than tag it here. The
fix is spread through this helper restack and is not a clean
cherry-pick for stable, and it pairs with free_tx_ltb() in patch 6 —
one defect, one patch. Putting that at the head of this
series makes it 16 patches, which exceeds the netdev single-series
limit. Sending it to net after this series keeps the posting within
the limit and makes the fix a clean standalone cherry-pick for stable.
Happy to restructure if you would rather have the fix in this posting.

>
> [ ... ]
>
>> +	netdev_dbg(netdev, "allocated %d RX queue(s) with %d entries each\n",
>> +		   adapter->num_rx_queues, rxq_entries);
> [ ... ]
>
>> +static void
>> +ibmveth_cleanup_rx_resources(struct ibmveth_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +	int i;
>> +
>> +	netdev_dbg(adapter->netdev, "cleaning up %d RX queue(s)\n",
>> +		   adapter->num_rx_queues);
> [Severity: Low]
> This isn't a bug, but adapter->num_rx_queues is declared as unsigned int in
> struct ibmveth_adapter in ibmveth.h, so should these two new debug messages
> in ibmveth_alloc_rx_queues() and ibmveth_cleanup_rx_resources() use %u
> rather than %d?  The same mismatch is still there at the end of the series
> where the value comes from ibmveth_get_num_rx_queues(), which also returns
> unsigned int.
Yes. Both use %u in v6.
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++) {
>> +		if (adapter->buffer_list_dma[i]) {
>> +			dma_unmap_single(dev, adapter->buffer_list_dma[i],
>> +					 4096, DMA_BIDIRECTIONAL);
>> +			adapter->buffer_list_dma[i] = 0;
>> +		}
>> +
>> +		if (adapter->rx_queue[i].queue_addr) {
>> +			dma_free_coherent(dev, adapter->rx_queue[i].queue_len,
>> +					  adapter->rx_queue[i].queue_addr,
>> +					  adapter->rx_queue[i].queue_dma);
>> +			adapter->rx_queue[i].queue_addr = NULL;
>> +		}
>> +
>> +		if (adapter->buffer_list_addr[i]) {
>> +			free_page((unsigned long)adapter->buffer_list_addr[i]);
>> +			adapter->buffer_list_addr[i] = NULL;
>> +		}
>> +	}
>> +}
>> +
>>   /* setup the initial settings for a buffer pool */
>>   static void ibmveth_init_buffer_pool(struct ibmveth_buff_pool *pool,
>>   				     u32 pool_index, u32 pool_size,
> [ ... ]
>
>> @@ -752,26 +890,12 @@ static int ibmveth_open(struct net_device *netdev)
>>   			ibmveth_free_buffer_pool(adapter,
>>   						 &adapter->rx_buff_pool[0][i]);
>>   	}
>> -out_unmap_filter_list:
>> -	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -
>>   out_free_tx_ltb:
>> -	while (--i >= 0) {
>> +	while (--i >= 0)
>>   		ibmveth_free_tx_ltb(adapter, i);
>> -	}
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does the fall-through from
> out_free_buffer_pools into out_free_tx_ltb leak the TX long term buffers?
>
> Both labels share the loop counter i.  On the buffer pool failure and the
> request_irq() failure paths, out_free_buffer_pools already runs
> while (--i >= 0) down to i == -1, so out_free_tx_ltb then evaluates
> --i == -2 and runs zero iterations:
>
> out_free_buffer_pools:
> 	while (--i >= 0) {
> 		...
> 	}
> out_free_tx_ltb:
> 	while (--i >= 0)
> 		ibmveth_free_tx_ltb(adapter, i);
>
> Every tx_ltb_ptr[] allocation plus its dma_map_single(DMA_TO_DEVICE) made
> by the earlier loop over real_num_tx_queues then stays around, and a later
> successful open overwrites the pointers and handles.
>
> The shared counter disappears at the end of the series, where TX LTB
> allocation moves into ibmveth_alloc_tx_resources() with its own err_free_ltbs
> unwind, so this only matters for this commit and its predecessors.
Yes. Pre-existing. Patch 4 stops the shared i; patch 6 gives TX its
own unwind.
>> -
>> -out_unmap_buffer_list:
>> -	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -out_free_queue_mem:
>> -	dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> -			  adapter->rx_queue[0].queue_addr,
>> -			  adapter->rx_queue[0].queue_dma);
>> +	ibmveth_cleanup_rx_resources(adapter);
>>   out_free_filter_list:
>> -	free_page((unsigned long)adapter->filter_list_addr);
>> -out_free_buffer_list:
>> -	free_page((unsigned long)adapter->buffer_list_addr[0]);
>> +	ibmveth_free_filter_list(adapter);
>>   out:
>>   	napi_disable(&adapter->napi[0]);
>>   	return rc;
> [Severity: High]
> This is a pre-existing issue, but can the hypervisor still be writing into
> these pages here?
>
> On the buffer pool allocation failure path in ibmveth_open():
>
> 		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;
> 		}
>
> there is no h_free_logical_lan() loop, unlike the request_irq() failure
> path just below it, so PHYP is still registered against the buffer list
> page, the RX ring and the filter list when control reaches
> ibmveth_cleanup_rx_resources() and ibmveth_free_filter_list(), which
> dma_unmap and free them.
>
> The consolidated helpers make that sequence easier to read but do not
> change it.  At the end of the series ibmveth_open() allocates the buffer
> pools before ibmveth_register_rx_queues() and every post-registration exit
> goes through ibmveth_free_all_queues() first, so is the ordering here worth
> fixing earlier, or is it intentionally left to the later "ibmveth: Add RX
> queue register helpers for MQ" patch?
It can, at this point in the series. Patch 6 issues
h_free_logical_lan() on the pool-fail path before RX pool DMA
teardown — that path never deregistered. Patch 7 then moves pool
alloc ahead of register_rx_queues() and routes every later error
exit through ibmveth_free_all_queues(), so PHYP has been dropped
before anything is unmapped. The staging is deliberate but the
intermediate state is real. both commit messages say so.
>> @@ -805,17 +928,8 @@ static int ibmveth_close(struct net_device *netdev)
>>   
>>   	ibmveth_update_rx_no_buffer(adapter);
>>   
>> -	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -	free_page((unsigned long)adapter->buffer_list_addr[0]);
>> -
>> -	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -	free_page((unsigned long)adapter->filter_list_addr);
>> -
>> -	dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> -			  adapter->rx_queue[0].queue_addr,
>> -			  adapter->rx_queue[0].queue_dma);
>> +	ibmveth_cleanup_rx_resources(adapter);
>> +	ibmveth_free_filter_list(adapter);
> [Severity: High]
> Can ibmveth_close() dereference a NULL buffer_list_addr[0] now?
>
> ibmveth_cleanup_rx_resources() clears the pointer after freeing it:
>
> 			free_page((unsigned long)adapter->buffer_list_addr[i]);
> 			adapter->buffer_list_addr[i] = NULL;
>
> while ibmveth_update_rx_no_buffer(), called just above, dereferences it
> unconditionally:
>
> 	__be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
>
> 	adapter->rx_no_buffer = be64_to_cpup(p);
>
> The baseline inline code in ibmveth_close() left the pointer stale, so the
> read did not fault.  With the helper clearing it, a failed re-open leaves
> the field NULL while the interface is still up:
>
> ibmveth_set_csum_offload() / ibmveth_set_tso() / ibmveth_change_mtu() /
> veth_pool_store()
>      ibmveth_close()
>      ibmveth_open()          <- fails, out_free_tx_ltb path runs
>          ibmveth_cleanup_rx_resources()   buffer_list_addr[0] = NULL
>
> and the next ndo_stop, or the queued ibmveth_reset() work reaching
> dev_close(), enters ibmveth_close() and reads through NULL + 4096 - 8.
>
> Later patches in the series appear to close this: ibmveth_close() becomes
> gated on adapter->opened, and ibmveth_update_rx_no_buffer() takes a queue
> index and returns early when buffer_list_addr[queue_index] is NULL.  Would
> it make sense to add that NULL check in this patch, since this is the commit
> that starts clearing the pointer?
Yes, for close(). Patch 5's opened gate stops that path.

It does not stop the other caller: replenish_task() from netpoll,
which consults neither opened nor RTNL. The guard that covers every

caller is in patch 10.

Thanks,

Mingming