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

[mingming cao]

On 10/8/26 2:08 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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?
*


Yes. Those paths already advance the ring, so the break is wrong.

The respin continues and counts the slot against the budget.

The re-arm can do the same thing on a normal burst. Once

napi_schedule() has succeeded, the poll will not return budget.

*
> [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.
*

Offset and length should be checked too. The respin drops a frame

that does not fit the pool. That has not shown up in the field, so

it has no additional Fixes: tag.

pw-bot: cr

Thanks,

Mingming

*