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.
> 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.
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.
> +
> + 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;
[ ... ]
