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

[Jakub Kicinski <kuba@kernel.org>]

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.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 8519cad503225..58a639a962a67 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -262,6 +272,7 @@ ibmveth_alloc_rx_queues(struct ibmveth_adapter *adapter, int rxq_entries)
>  		adapter->rx_queue[i].index = 0;
>  		adapter->rx_queue[i].num_slots = rxq_entries;
>  		adapter->rx_queue[i].toggle = 1;
> +		spin_lock_init(&adapter->rx_queue[i].replenish_lock);

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

[ ... ]

> @@ -883,21 +971,85 @@ static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
>  }
>  
>  /* replenish routine */
> -static void ibmveth_replenish_task(struct ibmveth_adapter *adapter)
> +static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
> +				   int queue_index)
>  {
> -	int i;
> +	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
> +	struct ibmveth_replenish_fail fail = {};
> +	unsigned long flags;
> +	int i, rc;
> +	int need_reset = 0;
> +	int batch_fallback = 0;
> +	int hcall_fail = 0;
> +
> +	if (queue_index >= adapter->num_rx_queues) {
> +		netdev_dbg(adapter->netdev,
> +			   "Skipping replenish for freed queue %d (num_queues=%d)\n",
> +			   queue_index, adapter->num_rx_queues);
> +		return;
> +	}
>  
>  	adapter->replenish_task_cycles++;

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

>  
>  /* empty and free ana buffer pool - also used to do cleanup in error paths */
> @@ -932,6 +1084,14 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
>  		kfree(pool->skbuff);
>  		pool->skbuff = NULL;
>  	}
> +
> +	/*
> +	 * Keep probe/sysfs geometry (active, size, buff_size, threshold).
> +	 * Clearing active here was a v3 ifdown/up regression: open skips
> +	 * !active pools, so reopen posted no RX buffers (TX OK, ARP/RX
> +	 * dead) at any queue count, including RX=8 with no -L.
> +	 */
> +	atomic_set(&pool->available, 0);
>  }

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

[ ... ]

> @@ -2127,34 +2340,41 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
> -			skb = ibmveth_rxq_get_buffer(adapter);
> +			skb = ibmveth_rxq_get_buffer(adapter, queue_index);
>  			if (unlikely(!skb))
>  				break;

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

[ ... ]

> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
> index fae3473cc4985..d02444d5b3b89 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.h
> +++ b/drivers/net/ethernet/ibm/ibmveth.h
> @@ -259,7 +261,8 @@ static inline long h_illan_attributes(unsigned long unit_address,
>  #define IBMVETH_DEFAULT_QUEUES 8U
>  #define IBMVETH_MAX_RX_QUEUES 1U
>  #define IBMVETH_DEFAULT_RX_QUEUES 1U
> -#define IBMVETH_MAX_RX_PER_HCALL 8U
> +#define IBMVETH_MAX_RX_REGULAR 8U
> +#define IBMVETH_MAX_RX_PER_HCALL 12U

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?

> @@ -301,6 +304,7 @@ struct ibmveth_rx_q {
>      dma_addr_t queue_dma;
>      u32        queue_len;
>      struct ibmveth_rx_q_entry *queue_addr;
> +	spinlock_t	replenish_lock;	/* per-queue buffer replenish */
>  };

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?