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

On 8/6/26 11:37 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.
> ---
> ibmveth: Refactor RX resource allocation for MQ RX bring-up
>
> This moves the filter list and per-queue RX ring allocation out of
> ibmveth_open() into four new helpers, and wires them into both the
> open error unwind and ibmveth_close().  Runtime behaviour is meant to
> stay single-queue since num_rx_queues is still 1.
Hi Jakub,

Thanks for the review.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 8e758362cb263..1007dd95cde02 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -151,6 +151,184 @@ 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;
>> +		return -ENOMEM;
>> +	}
> [Severity: Medium]
>
> Should this error path also clear adapter->filter_list_dma?  On failure
> dma_map_single() has stored DMA_MAPPING_ERROR, which is ~(dma_addr_t)0 and
> therefore non-zero, and only filter_list_addr is reset here.
>
Yes. In v5 I will clear `filter_list_dma` on mapping error, matching the
existing `buffer_list_dma` convention.

> The sibling helper added in the same patch does zero the cookie in the
> same situation:
>
> 	if (dma_mapping_error(dev, adapter->buffer_list_dma[i])) {
> 		...
> 		adapter->buffer_list_dma[i] = 0;
> 		goto err_cleanup;
> 	}
>
> so the two new helpers appear to disagree on the convention.

>> +
>> +	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;
>> +	}
> [Severity: Medium]
>
> Can this unmap the DMA_MAPPING_ERROR sentinel?  ibmveth_free_filter_list()
> treats any non-zero filter_list_dma as a live mapping, and the mapping-error
> branch of ibmveth_alloc_filter_list() leaves the sentinel behind.
>
> The state looks reachable without dev_close().  ibmveth_set_csum_offload(),
> ibmveth_set_tso(), ibmveth_change_mtu() and veth_pool_store() all call
> ibmveth_close() and then ibmveth_open() directly while netif_running() is
> still true:
>
> 	if (netif_running(dev)) {
> 		restart = 1;
> 		ibmveth_close(dev);
> 	}
> 	...
> 	if (restart)
> 		rc2 = ibmveth_open(dev);
>
> If that re-open fails at the dma_mapping_error() check in
> ibmveth_alloc_filter_list(), open() returns through the out: label without
> calling ibmveth_free_filter_list(), so the adapter is left with
> filter_list_addr == NULL and filter_list_dma == DMA_MAPPING_ERROR while the
> netdev is still marked running.  A later "ip link set dev ethX down" then
> reaches ndo_stop == ibmveth_close() -> ibmveth_free_filter_list() and unmaps
> that invalid address.
>
> On pseries the dma_iommu path catches this in iommu_free_check(), which emits
> a ratelimited "iommu_free: invalid entry" message and a WARN_ON(1) (a panic
> with panic_on_warn set), so no TCE state is changed, but the backtrace is
> still user triggerable with CAP_NET_ADMIN plus a mapping failure.
>
> Setting adapter->filter_list_dma = 0 in that error branch, matching
> ibmveth_alloc_rx_queues(), would make the guard here reliable.
>
> This is still present unchanged at the end of the series (a3781f4ae789), so
> a later patch does not appear to address it.

Yes. The failure path here is:
- `dma_map_single()` fails for the filter page
- open/reopen leaves `filter_list_addr = NULL`
- `filter_list_dma` is still left at `DMA_MAPPING_ERROR`
- a later free path sees a non-zero DMA handle and calls
   `dma_unmap_single()` on the sentinel value

That leads to the later bad unmap/WARN.

In v5 I will free the page and set `adapter->filter_list_dma = 0` on
mapping error while keeping `filter_list_addr = NULL`, so the free path
remains consistent and only unmaps real mappings.

>> +
>> +	if (adapter->filter_list_addr) {
>> +		free_page((unsigned long)adapter->filter_list_addr);
>> +		adapter->filter_list_addr = NULL;
>> +	}
>> +}
>> +
> [ ... ]
>
>> +		adapter->buffer_list_dma[i] =
>> +			dma_map_single(dev, adapter->buffer_list_addr[i],
>> +				       4096, DMA_BIDIRECTIONAL);
>> +		if (dma_mapping_error(dev, adapter->buffer_list_dma[i])) {
>> +			netdev_err(netdev,
>> +				   "unable to map buffer list for queue %d\n",
>> +				   i);
>> +			adapter->buffer_list_dma[i] = 0;
>> +			goto err_cleanup;
>> +		}
>> +
> [ ... ]
>
>> @@ -627,74 +805,34 @@ static int ibmveth_open(struct net_device *netdev)
>>   
>> -	rc = -ENOMEM;
>> -	adapter->buffer_list_addr[0] = (void *)get_zeroed_page(GFP_KERNEL);
>> -	if (!adapter->buffer_list_addr[0]) {
>> -		netdev_err(netdev, "unable to allocate list pages\n");
>> +	rc = ibmveth_alloc_filter_list(adapter);
>> +	if (rc)
>>   		goto out;
>> -	}
> [ ... ]
>
>> @@ -753,26 +889,12 @@ static int ibmveth_open(struct net_device *netdev)
>> +	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;
> [ ... ]

Thanks again for the review,
Mingming