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
*
