Re: [PATCH net-next v2 4/8] 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 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: NAPI list corruption and panic on bad RX correlator

Pre-existing issues:
- [Medium] Not introduced by this patch, but ibmveth_poll() takes the
  frame offset (flags & IBMVETH_RXQ_OFF_MASK, up to 0xFFFF) and length…

--

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

This patch moves RX correlator validation into one helper. The helper also
rejects pools that have no skbuff array, such as inactive pools. On a bad
slot, ibmveth_poll() now advances the RX ring, counts the drop in
rx_dropped and schedules a reset, and it logs with a ratelimited
netdev_err() instead of WARN_ON(). The patch also adds and extends KUnit
cases for these paths.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index d269599f5a990..3bac6cabbbb4c 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -1515,12 +1556,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: High]
Now that the ring is advanced before each of these break statements,
can ibmveth_poll() leave the loop with descriptors still pending and
go down the completion path before the budget is used up?

After the break, frames_processed is below budget, so poll calls
napi_complete_done(), re-enables the interrupt with h_vio_signal() and
then finds ibmveth_rxq_pending_buffer() true for the next slot. It
calls napi_schedule() and jumps to restart_poll without resetting
frames_processed.

If the restarted loop then uses the rest of the budget, ibmveth_poll()
returns budget after it has already rescheduled itself. The NAPI core
expects a driver that consumes its whole budget to leave the NAPI
state alone.

In net_rx_action(), __napi_poll() catches this and only prints the
"Budget exhausted after napi rescheduled" warning. However,
busy_poll_stop() calls __napi_schedule() whenever poll returns
budget. Would that put napi->poll_list on the per-CPU poll list a
second time and corrupt it?

A burst of traffic right behind a single bad correlator would be
enough to get there.

The !ibmveth_rxq_buffer_valid() recycle path already keeps looping
after it harvests a slot. Now that the ring is advanced on every
error, is there still a reason to break here rather than continue
with the next slot?

The same restart_poll sequence can already return budget today if a
burst arrives right after the interrupt is re-enabled. Should the
restart also avoid returning the full budget once napi_schedule()
has succeeded?

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The patch
hardens ibmveth_poll() against bad correlators from PHYP, so should the
frame offset and length from the same RX descriptor be checked as well?

ibmveth_poll() reads both values straight from the queue entry:

    int length = ibmveth_rxq_frame_length(adapter);
    int offset = ibmveth_rxq_frame_offset(adapter);

offset is flags_off & IBMVETH_RXQ_OFF_MASK, so it can be up to 0xFFFF.
length is a full be32. Neither is compared against
rx_buff_pool[pool].buff_size.

On the rx_copybreak path:

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

Can this read past the end of the RX buffer? If so, nearby heap memory
would be copied into a packet that is passed up the stack.

On the non-copybreak path:

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

If offset + length is too large, would this reach skb_over_panic() and
BUG() in softirq context?

As with the bad correlator case, this can only happen if PHYP reports an
offset or length larger than the buffer it was given.

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