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

[mingming cao <mmc@linux.ibm.com>]

On 7/14/26 6:10 AM, Simon Horman wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Fix MQ RX poll and shutdown hangs after queue resize
>
> This commit fixes polling hangs in the ibmveth driver after queue resize
> by skipping over bad correlators instead of retrying endlessly. It also
> ensures NAPI completes when the interface is down or being disabled.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 50a332ab83fd3..eb84b4e5f69fd 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -2959,8 +2978,23 @@ 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,
> [Severity: High]
> This isn't a bug introduced by this patch, but can the unvalidated packet
> lengths and offsets from hardware descriptors trigger an skb_over_panic()
> here?
yes
> Further down in ibmveth_poll(), the driver reads length and offset directly
> from the hardware receive descriptor and passes them to skb_reserve() and
> skb_put() on the reused SKB:
>
>      int length = ibmveth_rxq_frame_length(adapter, queue_index);
>      int offset = ibmveth_rxq_frame_offset(adapter, queue_index);
>      ...
>      } else {
>          rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, false);
>          if (unlikely(rc))
>              break;
>          skb_reserve(skb, offset);
>      }
>
>      skb_put(skb, length);
>
> If a faulty hypervisor provides an abnormally large length or offset,
> skb_put() will extend the tail past the end of the buffer, which triggers an
> skb_over_panic() and causes a crash.
>
> Should the driver validate that offset + length is within the known buffer
> size before modifying the boundaries?

**

*Agreed this needs guarding. I'll add in v4: validates*

*

offset + length against skb_tailroom() before skb_reserve/skb_put, and

skips bad correlators.

*

>
> [ ... ]
>
>> @@ -3034,8 +3068,11 @@ 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);
>>   		goto out;
>> +	}
> [Severity: High]
> Does calling napi_complete_done() here conflict with the NAPI core contract?
>
> If the polling loop exhausts its budget (frames_processed == budget), returning
> the full budget after calling napi_complete_done() might cause issues with the
> NAPI state machine. The NAPI core (__napi_poll) will operate on the instance
> assuming it still holds the NAPIF_STATE_SCHED bit.
>
> If napi_disable_pending() is true, the core calls napi_complete(), which will
> hit a WARN_ON_ONCE.
>
> If napi_disable_pending() is false but !netif_running() is true, the core will
> add the NAPI instance back to the poll_list. Since the driver cleared
> NAPIF_STATE_SCHED, a subsequent interrupt can trigger another napi_schedule(),
> causing a double list_add_tail() and potentially leading to list corruption.
*

Good catch — completing and then

returning a full budget is wrong. In v4 I'll complete on

shutdown/disable and return a value < budget (same as at the top of

poll), and leave the normal budget-exhausted path returning budget

without completing.


Thanks,

Mingming

*