Re: [PATCH net-next v5 03/15] 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 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().

[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.

[ ... ]

+	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.

+
+	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,

[ ... ]

[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.

-
-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?

[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?