Re: [PATCH net-next 4/7] ibmveth: step past bad RX correlators instead of spinning or oopsing

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new bad-slot handling in ibmveth_poll()
  (drivers/net/ethernet/ibm/ibmveth.c) neither charges consumed error…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but:
  ibmveth_poll() uses the PHYP-written descriptor length…

--

ibmveth: step past bad RX correlators instead of spinning or oopsing

This patch adds a helper that validates the RX correlator returned by
PHYP. The helper also rejects inactive pools that have no skbuff array.
On a bad slot, the RX ring now advances, the drop is counted in
rx_dropped, and a reset is scheduled. WARN_ON() is replaced with a
ratelimited netdev_err(), and the KUnit cases are extended to cover
these paths.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 77d3740cc52a6..b89ce389d951f 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -1468,6 +1503,7 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  	int frames_processed = 0;
>  	unsigned long lpar_rc;
>  	u16 mss = 0;
> +	int rc;
>  
>  restart_poll:
>  	while (frames_processed < budget) {
> @@ -1490,8 +1526,11 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  			__sum16 iph_check = 0;
>  
>  			skb = ibmveth_rxq_get_buffer(adapter);
> -			if (unlikely(!skb))
> +			if (unlikely(!skb)) {
> +				ibmveth_rxq_advance(adapter);
> +				netdev->stats.rx_dropped++;
>  				break;
> +			}

[Severity: Low]
The ring now moves past the bad slot. Should this consumed entry also
count against the NAPI budget, and should the loop continue here rather
than break?

frames_processed is not incremented here or in the two harvest error
branches below. The break goes to the tail of ibmveth_poll():

    ibmveth_replenish_task(adapter);

    if (frames_processed == budget)
        goto out;

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

    lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_ENABLE);
    ...
    if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
        lpar_rc = h_vio_signal(adapter->vdev->unit_address,
                               VIO_IRQ_DISABLE);
        goto restart_poll;
    }

With a run of N bad correlators, one poll call would then do N
replenish passes and 2N hcalls but report 0 work. This all runs on the
CPU where schedule_work() has just queued the reset.

A bad correlator never decrements pool->available. That means the burst
is only bounded by how many buffers are posted to PHYP, about 1024 with
the default active pools.

Documentation/networking/napi.rst says the budget "limits how many
packets driver can process in a single poll". Would it bound this work
to count the consumed error descriptors in frames_processed and use
continue instead of break?

>  
>  			/* if the large packet bit is set in the rx queue
>  			 * descriptor, the mss will be written by PHYP eight
> @@ -1515,12 +1554,19 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  				if (rx_flush)
>  					ibmveth_flush_buffer(skb->data,
>  						length + offset);
> -				if (unlikely(ibmveth_rxq_harvest_buffer(adapter, true)))
> +				rc = ibmveth_rxq_harvest_buffer(adapter, true);
> +				if (unlikely(rc)) {
> +					dev_kfree_skb_any(new_skb);
> +					netdev->stats.rx_dropped++;
>  					break;
> +				}
>  				skb = new_skb;
>  			} else {
> -				if (unlikely(ibmveth_rxq_harvest_buffer(adapter, false)))
> +				rc = ibmveth_rxq_harvest_buffer(adapter, false);
> +				if (unlikely(rc)) {
> +					netdev->stats.rx_dropped++;
>  					break;
> +				}
>  				skb_reserve(skb, offset);
>  			}

[Severity: Medium]
This isn't a bug introduced by this patch, but the correlator from PHYP
is now validated. Should the length and offset from the same descriptor
be checked as well?

ibmveth_rxq_frame_length() returns the raw be32 length as an int.
ibmveth_rxq_frame_offset() can return values up to IBMVETH_RXQ_OFF_MASK
(0xFFFF). ibmveth_poll() does not check either one against the source
pool's buff_size, which is 512 bytes for pool 0.

The copybreak branch does this:

    skb_copy_to_linear_data(new_skb,
                            skb->data + offset,
                            length);

If offset is close to 0xFFFF, can this read up to about 64 KiB past the
RX buffer and then hand that data up through napi_gro_receive()?

The non-copybreak branch does this:

    skb_reserve(skb, offset);
    ...
    skb_put(skb, length);

This path has no tailroom check. Could an oversized or negative length
reach skb_over_panic() -> BUG() in softirq?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790991039.git.mmc%40linux.ibm.com