Re: [PATCH net-next v4 14/14] ibmveth: Fix MQ RX poll and shutdown hangs after queue resize

[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: Fix MQ RX poll and shutdown hangs after queue resize

This changes ibmveth_poll() to skip RX slots whose correlator cannot be
mapped to a buffer pool instead of retrying them forever, completes NAPI
when the interface is going down, and adds a descriptor length check
before skb_put().  The WARN_ON plus adapter reset escalation in the
correlator validation paths is replaced by a rate-limited message.

No Link: tags present in the commit header.

A few questions below.

[Severity: Low]
Would it make sense to split this into separate patches with individual
Fixes: tags?  There appear to be four independent changes here:

  - skipping bad RX correlators and dropping the WARN_ON/reset escalation
  - completing NAPI when the interface is down or napi_disable is pending
  - validating the descriptor length against skb tailroom before skb_put()
  - changing the return value on the budget-exhausted shutdown path

There is no Fixes: tag, so stable maintainers cannot pick up the hang fix
without also picking up the policy change that removes the reset based
recovery, and the skb_put() hardening is not visible as a fix on its own.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index c99d8e8be7b37..09e06d86701a5 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1376,6 +1376,24 @@ ibmveth_free_single_rx_queue(struct ibmveth_adapter *adapter, int queue_idx)
>  	netdev_dbg(adapter->netdev, "Freed queue %d resources\n", queue_idx);
>  }
>  
> +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;
> +
> +	return pool < IBMVETH_NUM_BUFF_POOLS &&
> +	       index < adapter->rx_buff_pool[queue_index][pool].size;
> +}

[Severity: High]
This isn't a bug introduced by this patch, since the pre-existing WARN_ON
checks were also bounds-only, but can a correlator naming an inactive or
already freed pool still pass ibmveth_rxq_correlator_valid() and then
dereference a NULL array?

ibmveth_init_buffer_pool() sets pool->size for all pools regardless of
active state, while ibmveth_alloc_queue_buffer_pools() skips inactive
pools, and ibmveth_free_buffer_pool() clears the arrays without clearing
size or active:

	kfree(pool->free_map);
	pool->free_map = NULL;
	...
	if (pool->skbuff) {
		kfree(pool->skbuff);
		pool->skbuff = NULL;
	}

With the defaults in ibmveth.h:

	static int pool_count[] = { 256, 512, 256, 256, 256 };
	static int pool_active[] = { 1, 1, 0, 0, 0};

a correlator of pool=2, index=0 satisfies both conditions of the new
helper, and both users then index a NULL pointer before the !skb test can
help:

	return adapter->rx_buff_pool[queue_index][pool].skbuff[index];

and

	skb = adapter->rx_buff_pool[queue_index][pool].skbuff[index];

Should the helper also require pool->active and non-NULL skbuff/dma_addr/
free_map, or should ibmveth_free_buffer_pool() clear pool->size?  The same
window seems to apply to pools torn down by ethtool -L scale-down, an MTU
change, or sysfs deactivation.

[Severity: Low]
This is a pre-existing weakness rather than something new here, but the
helper only bounds-checks, so a stale correlator that happens to land in
range after an ethtool -L resize is treated as a live frame.  With
reuse=false the buffer can be handed to the stack while the hypervisor may
still own it, and with reuse=true the index is pushed onto free_map a
second time.  Is there any generation counter or per-queue tag available in
the correlator that could distinguish a stale descriptor from a current
one?

> +
> +static void ibmveth_rxq_advance(struct ibmveth_rx_q *rxq)
> +{
> +	if (++rxq->index == rxq->num_slots) {
> +		rxq->index = 0;
> +		rxq->toggle = !rxq->toggle;
> +	}
> +}
> +
>  /**
>   * ibmveth_remove_buffer_from_pool - remove a buffer from a pool
>   * @adapter: adapter instance
> @@ -1397,17 +1415,12 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>  	unsigned int free_index;
>  	struct sk_buff *skb;
>  
> -	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
> -	    WARN_ON(index >= adapter->rx_buff_pool[queue_index][pool].size)) {
> -		schedule_work(&adapter->work);
> +	if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator))
>  		return -EINVAL;
> -	}
>  
>  	skb = adapter->rx_buff_pool[queue_index][pool].skbuff[index];
> -	if (WARN_ON(!skb)) {
> -		schedule_work(&adapter->work);
> +	if (!skb)
>  		return -EFAULT;
> -	}

[Severity: Medium]
The schedule_work(&adapter->work) escalation is dropped here and in
ibmveth_rxq_get_buffer(), so nothing bounds the condition any more.  The
commit message describes the new skip policy but does not mention that all
recovery escalation is gone.

The previous reset performed a close/open cycle, which issued
h_free_logical_lan and re-registered the logical LAN, flushing every buffer
registration the hypervisor still held.  If the bad correlator exists
because PHYP still holds buffers from a pool that
ibmveth_free_buffer_pool() already unmapped and freed during an
ethtool -L resize:

	dma_unmap_single(&adapter->vdev->dev, pool->dma_addr[i],
			 pool->buff_size, DMA_FROM_DEVICE);
	dev_kfree_skb_any(skb);

can the hypervisor keep writing into those freed pages indefinitely now
that the driver only logs and advances?

Separately, this also folds together two different classes: -EINVAL for an
out-of-range correlator, and -EFAULT where pool and index are in range but
skbuff[index] is NULL, which indicates driver/hypervisor state desync.  Is
silently skipping the -EFAULT case intended?

And if the descriptor's correlator belongs to a different queue's pool, the
skip never reclaims that queue's slot, so that pool's available count stays
inflated and ibmveth_replenish_task() stops replenishing it:

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

>  
>  	/* if we are going to reuse the buffer then keep the pointers around
>  	 * but mark index as available. replenish will see the skb pointer and
> @@ -1452,11 +1465,8 @@ ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter,
>  	unsigned int pool = correlator >> 32;
>  	unsigned int index = correlator & 0xffffffffUL;
>  
> -	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
> -	    WARN_ON(index >= adapter->rx_buff_pool[queue_index][pool].size)) {
> -		schedule_work(&adapter->work);
> +	if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator))
>  		return NULL;
> -	}
>  
>  	return adapter->rx_buff_pool[queue_index][pool].skbuff[index];
>  }
> @@ -1483,14 +1493,15 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>  
>  	cor = rxq->queue_addr[rxq->index].correlator;
>  	rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
> -	if (unlikely(rc))
> +	if (unlikely(rc)) {
> +		if (rc == -EINVAL || rc == -EFAULT)
> +			goto advance;
>  		return rc;
> -
> -	if (++rxq->index == rxq->num_slots) {
> -		rxq->index = 0;
> -		rxq->toggle = !rxq->toggle;
>  	}
>  
> +advance:
> +	ibmveth_rxq_advance(rxq);
> +
>  	return 0;
>  }

[Severity: Low]
Can ibmveth_rxq_harvest_buffer() still return non-zero after this change?
ibmveth_remove_buffer_from_pool() has exactly three returns: -EINVAL,
-EFAULT and 0, and both error codes now go to the advance label, so
"return rc;" looks unreachable.

That would make all five "if (unlikely(rc)) break;" checks in
ibmveth_poll(), including the two added by this patch, dead code, along
with the harvest failure path in ibmveth_drain_rx_queue():

	rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
	if (rc) {
		netdev_err(netdev,
			   "Failed to harvest buffer from queue %d during drain: %d\n",
			   queue_index, rc);
		break;
	}
	drained++;

Should unmappable slots encountered during drain still be counted as
drained++ with no diagnostic?  The kernel-doc above harvest still documents
"other - non-zero return from ibmveth_remove_buffer_from_pool", which no
longer matches the implementation.

>  
> @@ -3092,11 +3103,19 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  	if (WARN_ON(queue_index < 0 || queue_index >= adapter->num_rx_queues))
>  		return 0;
>  
> +	if (!netif_running(netdev) || napi_disable_pending(napi)) {
> +		napi_complete_done(napi, 0);
> +		return 0;
> +	}
> +
>  	if (adapter->rx_qstats)
>  		adapter->rx_qstats[queue_index].polls++;
>  
>  restart_poll:
>  	while (frames_processed < budget) {
> +		if (!netif_running(netdev) || napi_disable_pending(napi))
> +			break;
> +
>  		if (!ibmveth_rxq_pending_buffer(adapter, queue_index))
>  			break;
>  

[ ... ]

> @@ -3126,8 +3145,45 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  			__sum16 iph_check = 0;
>  
>  			skb = ibmveth_rxq_get_buffer(adapter, queue_index);
> -			if (unlikely(!skb))
> -				break;
> +			if (unlikely(!skb)) {
> +				if (net_ratelimit())
> +					netdev_err(netdev,
> +						   "bad correlator on queue %d, skipping slot\n",
> +						   queue_index);

[ ... ]

> +
> +			if (unlikely((unsigned int)offset +
> +				     (unsigned int)length >
> +				     skb_tailroom(skb))) {

[Severity: High]
This is not a regression from this patch, since there was no length check
at all before, but can this check be bypassed by 32-bit wraparound?

Both operands are cast to unsigned int, so the sum is modulo 2^32.  Unlike
offset, length is not masked; ibmveth_rxq_frame_length() returns the whole
descriptor field:

	return be32_to_cpu(rxq->queue_addr[rxq->index].length);

and in ibmveth.h that field is a full __be32 written by the hypervisor,
while only offset gets IBMVETH_RXQ_OFF_MASK (0x0000FFFF).

For a corrupted length like 0xFFFFFFF0 with a small offset, the addition
wraps to a small value and the guard does not fire.  The copybreak test
compares against "static unsigned int rx_copybreak", so 0xFFFFFFF0 is not
less than rx_copybreak and no copy skb is allocated, and execution reaches:

	skb_put(skb, length);

which trips skb_over_panic() -> BUG().  Would an overflow-safe form work
better here, for example:

	if (offset > tailroom || length > tailroom - offset)

or validating length against pool->buff_size?

[ ... ]

> @@ -3206,8 +3262,14 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  
>  	ibmveth_replenish_task(adapter, queue_index);
>  
> -	if (frames_processed == budget)
> +	if (frames_processed == budget) {
> +		if (!netif_running(netdev) || napi_disable_pending(napi)) {
> +			napi_complete_done(napi, frames_processed);
> +			/* After complete_done, must not return full budget. */
> +			return frames_processed ? frames_processed - 1 : 0;
> +		}
>  		goto out;
> +	}
>  
>  	if (!napi_complete_done(napi, frames_processed))
>  		goto out;
> @@ -3225,6 +3287,8 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  	}
>  
>  	if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
> +	    netif_running(netdev) &&
> +	    !napi_disable_pending(napi) &&
>  	    napi_schedule(napi)) {
>  		lpar_rc = ibmveth_disable_irq(adapter, queue_index);
>  		WARN_ON(lpar_rc != H_SUCCESS);

[Severity: High]
This isn't a bug introduced by this patch, as the
napi_complete_done() -> ibmveth_enable_irq() -> ibmveth_rxq_pending_buffer()
sequence is unchanged, but do the new guards actually close the shutdown
window the commit message describes?

The new mid-loop break also lands here with frames_processed < budget, so
the quiesce case still reaches:

	if (!napi_complete_done(napi, frames_processed))
		goto out;

Clearing NAPI_STATE_SCHED there is what napi_disable() waits for, so
ibmveth_close() can proceed past ibmveth_cleanup_rx_interrupts() into
ibmveth_cleanup_rx_resources():

	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;

while the still running poll executes:

	lpar_rc = ibmveth_enable_irq(adapter, queue_index);

re-arming the PHYP interrupt that close just masked, and then evaluates
ibmveth_rxq_pending_buffer(), which reads
rxq->queue_addr[rxq->index].flags_off - a NULL dereference or a read of
freed coherent memory.  ibmveth_close() has no synchronize_net() between
napi_disable()/free_irq() and the frees, unlike the scale-down path.

Note the new netif_running()/napi_disable_pending() terms are evaluated
after ibmveth_rxq_pending_buffer() in the same condition, so they cannot
prevent that access; they only suppress the following
ibmveth_disable_irq().

Related question on that suppression: in the window where
__LINK_STATE_START is already cleared by __dev_close_many() but
NAPI_STATE_DISABLE is not yet set, the pre-patch code re-masked PHYP via
the napi_schedule() branch.  With the new guards, poll now returns leaving
delivery unmasked into the napi_disable()/free_irq() window, and the
interrupt handler does not mask either when napi_schedule_prep() fails:

	if (napi_schedule_prep(napi)) {
		lpar_rc = ibmveth_disable_irq(adapter, qindex);
		WARN_ON(lpar_rc != H_SUCCESS);
		__napi_schedule(napi);
	}

Can that leave the queue interrupt storming until free_irq()?

[ ... ]