Re: [PATCH net-next v5 08/15] ibmveth: Add queue-aware RX buffer submit helper for MQ

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: Add queue-aware RX buffer submit helper for MQ

This adds ibmveth_add_logical_lan_buffers() to pick between the legacy
single/multi buffer hcalls and the new per-queue H_ADD_LOGICAL_LAN_BUFFERS_QUEUE
hcall, threads queue_index through the RX replenish/poll helpers, and
introduces a per-queue replenish_lock plus deferred (post-unlock) failure
logging and reset scheduling.

A few questions below about the new locking scope and about some of the
error paths that changed behaviour.

[ ... ]

[Severity: Medium]
Is it safe to re-initialize replenish_lock here, given that
ibmveth_alloc_rx_queues() runs on every ndo_open?

The driver restarts itself directly in several places, guarded only by
netif_running(), for example ibmveth_set_csum_offload():

	if (netif_running(dev)) {
		restart = 1;
		ibmveth_close(dev);
	}
	...
	if (restart)
		rc2 = ibmveth_open(dev);

veth_pool_store() and the MTU change path do the same. Because
__LINK_STATE_START is never cleared on these paths, netpoll_poll_disable()
is never called, and netpoll_poll_dev() only bails out on !npinfo or a held
dev_lock (dev_lock is taken solely by netpoll_poll_disable() from
__dev_open/__dev_close_many).

So can this sequence happen while netconsole is configured on this device?

CPU0 (printk -> netconsole)
  ibmveth_poll_controller()
    ibmveth_replenish_task(adapter, 0)
      spin_lock_irqsave(&rxq->replenish_lock, flags);

CPU1 (ethtool -K restart)
  ibmveth_open()
    ibmveth_alloc_rx_queues()
      spin_lock_init(&adapter->rx_queue[0].replenish_lock);

That resets a held spinlock to unlocked, and the later unlock on CPU0 runs
against re-initialized state.

[ ... ]

-/* replenish the buffers for a pool.  note that we don't need to
- * skb_reserve these since they are used for incoming...
+/**
+ * ibmveth_add_logical_lan_buffers - Add receive buffers to hypervisor
+ * @adapter: ibmveth adapter structure
+ * @descs: array of buffer descriptors to add
+ * @filled: number of valid descriptors in the array
+ * @buff_size: size of each buffer (multi-queue mode only)
+ * @queue_index: RX queue index
+ *
+ * Return: hypervisor return code
  */
-static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
-					  struct ibmveth_buff_pool *pool)
+static long ibmveth_add_logical_lan_buffers(struct ibmveth_adapter *adapter,
+					    union ibmveth_buf_desc *descs,
+					    int filled,
+					    unsigned long buff_size,
+					    int queue_index)
+{
+	struct vio_dev *vdev = adapter->vdev;
+	unsigned long rc;
+
+	if (adapter->multi_queue) {
+		unsigned long buffersznum = (buff_size << 32) | filled;
+		unsigned long ioba[IBMVETH_MAX_RX_PER_HCALL / 2] = {0};
+		unsigned long handle = adapter->queue_handle[queue_index];
+		int i;

[Severity: Low]
This call site passes adapter->queue_handle[queue_index], which includes
queue 0, but the kernel-doc of h_add_logical_lan_buffers_queue() in
ibmveth.h describes it as "Add buffers to subordinate queue" with
"@queue_handle: Queue handle from h_register_logical_lan_queue()", and its
Return section lists only H_SUCCESS/H_PARAMETER/H_HARDWARE.

The queue 0 handle comes from h_register_logical_lan_with_handle(), whose
own doc says the handle is "needed in multi-queue mode to use
h_add_logical_lan_buffers_queue() for all queues including queue 0", and
ibmveth_replenish_buffer_pool() has dedicated recovery for H_FUNCTION from
this hcall.

Could the h_add_logical_lan_buffers_queue() kernel-doc be updated to cover
the primary queue handle and H_FUNCTION?

[ ... ]

+		rc = h_add_logical_lan_buffers_queue(vdev->unit_address,
+						     handle,
+						     buffersznum,
+						     ioba[0], ioba[1], ioba[2],
+						     ioba[3], ioba[4], ioba[5]);
+		adapter->hcall_stats.add_bufs_queue++;
+	} else if (filled == 1) {
+		rc = h_add_logical_lan_buffer(vdev->unit_address,
+					      descs[0].desc);
+		adapter->hcall_stats.add_buf++;
+	} else {
+		rc = h_add_logical_lan_buffers(vdev->unit_address,
+					       descs[0].desc, descs[1].desc,
+					       descs[2].desc, descs[3].desc,
+					       descs[4].desc, descs[5].desc,
+					       descs[6].desc, descs[7].desc);
+		adapter->hcall_stats.add_bufs++;
+	}

[Severity: Low]
IBMVETH_MAX_RX_PER_HCALL changes from 8 to 12 in this patch, which resizes
descs[] and correlators[] in ibmveth_replenish_buffer_pool() to 12, but this
legacy branch still hands only descs[0..7] to the 8-descriptor
h_add_logical_lan_buffers(), and the caller then accounts all "filled"
descriptors as posted (consumer_index advanced, pool->available
incremented).

The combination needed to hit that (multi_queue == 0 with
rx_buffers_per_hcall > 8) does not appear reachable, since probe now sets
IBMVETH_MAX_RX_REGULAR (8) or 1, and later in the series
ibmveth_apply_mq_fallback() clamps to IBMVETH_MAX_RX_REGULAR.

Would a clamp on filled here, or a BUILD_BUG_ON tying the legacy branch to
IBMVETH_MAX_RX_REGULAR, be worth adding so the arity contract is enforced
rather than implied?

Also, the 12-address MQ packing (ioba[4], ioba[5]) cannot be exercised at
this commit, since probe caps the batch at 8:

-		adapter->rx_buffers_per_hcall = IBMVETH_MAX_RX_PER_HCALL;
+		adapter->rx_buffers_per_hcall = IBMVETH_MAX_RX_REGULAR;

[ ... ]

+/* Replenish the buffers for a pool.
+ * Caller must hold the per-queue replenish_lock. Do not printk here —
+ * netconsole on the same device can re-enter replenish_task.
+ */
+static int ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
+					 struct ibmveth_buff_pool *pool,
+					 int queue_index,
+					 struct ibmveth_replenish_fail *fail)
 {

[Severity: High]
The "do not printk here" invariant is documented for this function, but can
code called from inside the critical section still printk indirectly?

dma_map_single() in the fill loop goes through iommu_map_page() on pseries,
and the driver passes attrs = 0:

arch/powerpc/kernel/iommu.c:iommu_map_page() {
	...
		if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())  {
			dev_info(dev, "iommu_alloc failed, tbl %p vaddr %p npages %d\n", ...);
	...
}

That is exactly the condition replenish hits (TCE table or CMO entitlement
exhausted under pressure). With netconsole on this device, the printk
recurses:

	printk -> write_msg -> netpoll_send_udp -> __netpoll_send_skb
	  -> netpoll_poll_dev -> ibmveth_poll_controller
	    -> ibmveth_replenish_task -> spin_lock_irqsave(&rxq->replenish_lock)

Since the lock is already held on this CPU with interrupts disabled, does
this self-deadlock? The same re-entry also reaches
ibmveth_rxq_harvest_buffer() -> ibmveth_remove_buffer_from_pool(), which now
takes the same lock.

[ ... ]

[Severity: Medium]
Several adapter-global fields are updated while holding only a per-queue
lock, or none at all. replenish_task_cycles++ here is outside the lock, and
inside ibmveth_replenish_buffer_pool() / ibmveth_add_logical_lan_buffers()
the following are adapter-wide:

	batch = adapter->rx_buffers_per_hcall;
	...
	adapter->rx_buffers_per_hcall = 1;
	adapter->hcall_stats.add_buf/add_bufs/add_bufs_queue++;
	adapter->replenish_add_buff_success/failure, replenish_no_mem

Two queues replenishing on different CPUs hold different replenish_lock
instances, so the intersection of locks protecting these is empty. Can this
lose or tear the ethtool-visible hcall_stats and replenish_* counters? Today
it is already racy between ibmveth_poll_controller() and NAPI for
replenish_task_cycles++, which is incremented before the lock is taken.

 
-	for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
-		struct ibmveth_buff_pool *pool = &adapter->rx_buff_pool[0][i];
+	spin_lock_irqsave(&rxq->replenish_lock, flags);
 
-		if (pool->active &&
-		    (atomic_read(&pool->available) < pool->threshold))
-			ibmveth_replenish_buffer_pool(adapter, pool);
+	for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
+		struct ibmveth_buff_pool *pool =
+			&adapter->rx_buff_pool[queue_index][i];
+
+		if (pool->active && pool->free_map &&
+		    (atomic_read(&pool->available) < pool->threshold)) {
+			rc = ibmveth_replenish_buffer_pool(adapter, pool,
+							   queue_index, &fail);

[Severity: High]
How long can interrupts stay disabled here?

The lock is taken with spin_lock_irqsave() around the whole five-pool walk,
and ibmveth_replenish_buffer_pool() loops "while (remaining > 0)" doing
netdev_alloc_skb() plus dma_map_single() per buffer, then a hypercall per
batch. With pool_active { 1, 1, 0, 0, 1 } and pool_count { 256, 512, 256,
256, 256 }, a cold start posts roughly 1024 buffers, including 256 of
buff_size 64 KB.

ibmveth_open() hits this deterministically:

	for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
		ibmveth_replenish_task(adapter, i);

So every ifup does on the order of a thousand GFP_ATOMIC allocations
(some high order), a thousand IOMMU mappings and ~128 hypercalls with local
interrupts off. Previously this ran with interrupts enabled and no lock.

Can that trip the hard lockup watchdog or produce RCU stalls, and does
disabling BH/IRQ progress here also make the GFP_ATOMIC allocations more
likely to fail exactly when memory is tight? Since
ibmveth_remove_buffer_from_pool() now takes the same lock per received
packet, the NAPI consumer is also blocked for the whole region.

The changelog notes "Defer adapter-global counter atomics and irqsave
critical-section shorten to cover follow-up" - would it be better to shorten
the region in this patch, since as merged the region exists?

[ ... ]

+out_unlock:
 	ibmveth_update_rx_no_buffer(adapter);

[Severity: Low]
This helper is now called per queue, but ibmveth_update_rx_no_buffer() still
reads queue 0's buffer-list page:

	__be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;

Each RX queue has its own buffer-list page whose last 8 bytes hold that
queue's no-buffer drop count, so rx_no_buffer would under-report queues
1..N. This is addressed later in the series, where the helper takes
queue_index and uses buffer_list_addr[queue_index], and MQ is only turned on
by a later patch, so no wrong-page read looks reachable at this commit.
Would it be cleaner to make the helper queue-aware in the same patch that
makes its caller queue-aware?

+
+	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
+
+	/* Log and schedule reset only after dropping replenish_lock. */
+	if (need_reset == IBMVETH_REPLENISH_RESET_MAP) {
+		netdev_info(adapter->netdev,
+			    "Invalid RX free_map entry on queue %d, reset\n",
+			    queue_index);
+		schedule_work(&adapter->work);

[Severity: Low]
This isn't a bug introduced by this patch, but the new schedule_work() sites
add more ways to arm adapter->work from NAPI context, and ibmveth_remove()
still cancels before it stops the device:

	cancel_work_sync(&adapter->work);
	...
	unregister_netdev(netdev);
	free_netdev(netdev);

RX activity between cancel_work_sync() and the ndo_close() done by
unregister_netdev() can re-queue the work, which may then run after
free_netdev(). The ordering is swapped later in this series
(unregister_netdev() first, then cancel_work_sync()), with a comment
describing exactly this window.

+	} else if (need_reset == IBMVETH_REPLENISH_RESET_MQ) {
+		dev_err_ratelimited(&adapter->netdev->dev,
+				    "MQ buffer add H_FUNCTION (q=%d, batch=%u), reset\n",
+				    queue_index, fail.batch);
+		schedule_work(&adapter->work);
+	}
+
+	if (batch_fallback)
+		dev_warn_ratelimited(&adapter->netdev->dev,
+				     "Legacy batch add H_FUNCTION (batch=%u), fallback\n",
+				     fail.batch);
+
+	if (hcall_fail)
+		dev_warn_ratelimited(&adapter->netdev->dev,
+				     "RX %s failed: filled=%u, rc=%lu, batch=%u\n",
+				     adapter->multi_queue ?
+				     "h_add_logical_lan_buffers_queue" :
+				     (fail.batch == 1 ?
+				      "h_add_logical_lan_buffer" :
+				      "h_add_logical_lan_buffers"),
+				     fail.filled, fail.lpar_rc, fail.batch);
 }

[Severity: Low]
Does this pick the wrong hcall name? The dispatch in
ibmveth_add_logical_lan_buffers() selects on filled, not batch:

	} else if (filled == 1) {
		rc = h_add_logical_lan_buffer(vdev->unit_address, descs[0].desc);

If batch is 8 and the fill loop breaks early after one buffer
(netdev_alloc_skb() or dma_mapping_error() failure), the failing call was
h_add_logical_lan_buffer(), but this prints h_add_logical_lan_buffers.
Should the selector be fail.filled == 1?

Separately, one "struct ibmveth_replenish_fail fail" and the
batch_fallback/hcall_fail flags are shared by all five pool iterations, so a
later pool's failure overwrites an earlier pool's rc/filled/batch, and one
log line can mix values from two pools.

[Severity: Low]
This comment describes a regression that does not exist in the tree: no
version of ibmveth_free_buffer_pool() ever cleared pool->active, and this
hunk is additive only. The same applies to the new comment in
ibmveth_rxq_correlator_valid() ("free_buffer_pool() clears skbuff but used
to leave size/active set").

References to "v3" and to lab options like "RX=8 with no -L" aren't
meaningful to a reader of mainline. Could the comments be rewritten in terms
of the code as it stands, and could the functional change here
(atomic_set(&pool->available, 0)) be mentioned in the commit message? Since
ibmveth_alloc_buffer_pool() already zeroes available on reallocation, it is
hard to tell from the patch whether this is a fix or a no-op.

[Severity: High]
This is a pre-existing race, but the new locking makes it look handled while
leaving it open. ibmveth_free_buffer_pool() kfrees pool->free_map,
pool->skbuff and pool->dma_addr without taking the per-queue
replenish_lock, while ibmveth_replenish_task() checks and uses those arrays
under that lock:

		if (pool->active && pool->free_map &&
		    (atomic_read(&pool->available) < pool->threshold)) {

So the check is a TOCTOU: replenish touches the pool arrays with
{rxq->replenish_lock} held, the free path touches and frees them with no
lock, giving an empty lock intersection.

Reachable through a direct close, which does not run
netpoll_poll_disable():

CPU0 (printk -> netconsole)
  ibmveth_poll_controller() -> ibmveth_replenish_task(adapter, 0)
    passes "pool->free_map != NULL", then dereferences
    pool->free_map / pool->skbuff / pool->dma_addr

CPU1 (ethtool -K / veth_pool_store / MTU change)
  ibmveth_close() -> ibmveth_free_buffer_pools()
    -> kfree(pool->free_map)

Can that produce a use-after-free of those arrays, and post freed buffers to
the hypervisor? Should ibmveth_free_buffer_pool() take replenish_lock (or
should the pool teardown otherwise be serialized against replenish)?

[ ... ]

+static bool ibmveth_rxq_correlator_valid(struct ibmveth_adapter *adapter,
+					 int queue_index, u64 correlator)
+{
+	unsigned int pool = correlator >> 32;
+	unsigned int index = correlator & 0xffffffffUL;
+	struct ibmveth_buff_pool *bpool;
+
+	if (pool >= IBMVETH_NUM_BUFF_POOLS)
+		return false;
+
+	bpool = &adapter->rx_buff_pool[queue_index][pool];
+
+	/* init_buffer_pool() sets size for inactive pools; free_buffer_pool()
+	 * clears skbuff but used to leave size/active set. Require a live
+	 * pool with allocated arrays before indexing.
+	 */
+	if (!bpool->active || !bpool->skbuff || !bpool->free_map)
+		return false;
+
+	return index < bpool->size;
+}

[Severity: Medium]
Do the in-tree KUnit cases still pass with the added
"!bpool->free_map" precondition? Neither
ibmveth_remove_buffer_from_pool_test() nor ibmveth_rxq_get_buffer_test()
allocates free_map - both only kunit_kcalloc() pool->skbuff.

That makes validation fail before the skbuff[index] NULL check, so the two
-EFAULT expectations should now receive -EINVAL:

	correlator = (u64)0 | 0;
	pool->skbuff[0] = NULL;
	KUNIT_EXPECT_EQ(test, -EFAULT,
			ibmveth_remove_buffer_from_pool(adapter,
							correlator, 0, false));

and the positive expectation in ibmveth_rxq_get_buffer_test() should receive
NULL:

	pool->skbuff[0] = skb;
	adapter->rx_queue[0].queue_addr[0].correlator = (u64)0 << 32 | 0;
	KUNIT_EXPECT_PTR_EQ(test, skb, ibmveth_rxq_get_buffer(adapter, 0));

The index-out-of-range expectations also pass vacuously now, so the bound
check they were written for is no longer covered. The patch updates the
kernel-doc of both tests but not the fixtures - should the fixtures also
allocate free_map?

[ ... ]

  * Return:
  * * %0       - success
  * * %-EINVAL - correlator maps to pool or index out of range
  * * %-EFAULT - pool and index map to null skb
  */
 static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
-					   u64 correlator, bool reuse)
+					   u64 correlator, int queue_index,
+					   bool reuse)
 {

[Severity: Low]
The documented meaning of -EINVAL is now incomplete: via
ibmveth_rxq_correlator_valid() this function also returns -EINVAL for an
inactive pool and for a pool whose skbuff/free_map arrays are not
allocated, which is a different condition from "out of range". The new
ibmveth_rxq_harvest_buffer() kernel-doc reinforces the narrow reading by
telling callers that -EINVAL/-EFAULT mean a "corrupt slot" safe to skip.
Could the return-code documentation be extended to cover the not-live-pool
case?

+	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
 	unsigned int pool  = correlator >> 32;
 	unsigned int index = correlator & 0xffffffffUL;
 	unsigned int free_index;
 	struct sk_buff *skb;
+	unsigned long flags;
+	int rc = 0;
 
-	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
-	    WARN_ON(index >= adapter->rx_buff_pool[0][pool].size)) {
-		schedule_work(&adapter->work);
-		return -EINVAL;
+	spin_lock_irqsave(&rxq->replenish_lock, flags);
+
+	if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator)) {
+		rc = -EINVAL;
+		goto out_unlock;
 	}
 
-	skb = adapter->rx_buff_pool[0][pool].skbuff[index];
-	if (WARN_ON(!skb)) {
-		schedule_work(&adapter->work);
-		return -EFAULT;
+	skb = adapter->rx_buff_pool[queue_index][pool].skbuff[index];
+	if (!skb) {
+		rc = -EFAULT;
+		goto out_unlock;
 	}

[ ... ]

 static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
-				      bool reuse)
+				      int queue_index, bool reuse)
 {
+	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
 	u64 cor;
 	int rc;
 
-	struct ibmveth_rx_q *rxq = &adapter->rx_queue[0];
-
 	cor = rxq->queue_addr[rxq->index].correlator;
-	rc = ibmveth_remove_buffer_from_pool(adapter, cor, reuse);
-	if (unlikely(rc))
+	rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
+	if (unlikely(rc)) {
+		/* Skip a corrupt slot without claiming pool ownership. */
+		if (rc == -EINVAL || rc == -EFAULT)
+			ibmveth_rxq_advance(rxq);
 		return rc;
-
-	if (++adapter->rx_queue[0].index == adapter->rx_queue[0].num_slots) {
-		adapter->rx_queue[0].index = 0;
-		adapter->rx_queue[0].toggle = !adapter->rx_queue[0].toggle;
 	}

[Severity: Medium]
The reset that previously repaired this state is gone -
ibmveth_remove_buffer_from_pool() no longer calls
schedule_work(&adapter->work) for -EINVAL/-EFAULT, and here the ring is just
advanced. Does that permanently retire the buffer the hypervisor already
consumed?

On this path free_map does not regain the index, the
atomic_dec(&pool->available) is skipped, and pool->skbuff[index] plus its
DMA mapping stay owned by nobody.

Since replenish is gated on available < threshold (size * 7 / 8):

		if (pool->active && pool->free_map &&
		    (atomic_read(&pool->available) < pool->threshold)) {

after roughly size/8 such events, does replenish stop for that pool
permanently, with the link still up and no recovery short of ifdown/up?

The poll "recycling invalid buffer" path and ibmveth_drain_rx_queue() both
swallow these errors without escalating:

	/* -EINVAL/-EFAULT already advanced past the slot. */
	if (rc == -EINVAL || rc == -EFAULT) {
		drained++;
		continue;
	}

At the end of the series only the get_buffer path escalates via
ibmveth_poll_skip_bad_correlator(), and that code's own comment ("Residual
stale slot after resize") suggests stale correlators do occur. Should the
recycle and drain paths also reconcile available or schedule a reset?

[ ... ]

[Severity: Low]
This isn't a bug introduced by this patch, but breaking out here leaves
rxq->index and rxq->toggle untouched, so the descriptor stays pending. The
tail of ibmveth_poll() then completes NAPI, re-enables the IRQ, and:

	if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
	    napi_schedule(napi)) {
		rc = ibmveth_disable_irq(adapter, queue_index);
		WARN_ON(rc);
		goto restart_poll;
	}

Can that spin on the same slot in softirq context when a VALID descriptor
has a correlator that fails validation, or whose pool->skbuff[index] is
NULL? The changelog says this lands with the poll harden patch ("Bad
queue_index poll path: napi_complete before return lands with poll
harden"), and later in the series
ibmveth_poll_skip_bad_correlator() schedules a reset and advances the ring.

[ ... ]

The change of IBMVETH_MAX_RX_PER_HCALL from 8 to 12 is described in the
commit message only as "up to 12 buffers" for the MQ hcall - could the
commit message also state that the existing constant is being redefined, so
that the descs[]/correlators[] resize in ibmveth_replenish_buffer_pool() and
the new IBMVETH_MAX_RX_REGULAR use in probe are easy to follow?

This isn't a bug, but the new member uses tabs while the surrounding members
in this struct use the older space-based alignment - was the mixed style
intentional?