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

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.

[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;
+		}
+

[ ... ]

[ ... ]

[ ... ]