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

[Simon Horman <horms@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.
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?

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?

[ ... ]

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