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] This patch adds a frame-bound check to ibmveth_poll()
(drivers/net/ethernet/ibm/ibmveth.c, line 1555).
--
ibmveth: step past bad RX correlators instead of spinning or oopsing
This patch changes ibmveth_poll() to advance the RX ring past a bad
correlator, so it no longer spins on the same slot or dereferences the
NULL skbuff array of an inactive pool. It also drops frames whose
offset + length does not fit the pool buffer, or which are shorter than
an Ethernet header. Correlator validation moves into one helper that
logs a ratelimited netdev_err() and schedules a reset.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index cee0e9783b2ab..919b5831d875c 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -1469,31 +1510,60 @@ static int ibmveth_poll(struct napi_struct *napi, int
> budget)
[ ... ]
> + pool = correlator >> 32;
> + room = min_t(unsigned int, skb_tailroom(skb),
> + adapter->rx_buff_pool[pool].buff_size);
> + off = offset;
> + len = length;
> + if (unlikely(len < ETH_HLEN || off >= room ||
> + len > room - off)) {
> + if (net_ratelimit())
> + netdev_err(netdev,
> + "bad RX frame offset %u
> length %u (buffer %u), dropping\n",
> + off, len, room);
> + ibmveth_rxq_harvest_buffer(adapter, correlator,
> + true);
> + netdev->stats.rx_dropped++;
^^^^
[Severity: Low]
Should these frames be counted in rx_length_errors and rx_errors rather
than rx_dropped?
The struct rtnl_link_stats64 documentation in
include/uapi/linux/if_link.h describes rx_length_errors as:
@rx_length_errors: Number of packets dropped due to invalid length.
and says rx_errors "must include events counted by @rx_length_errors".
rx_dropped is documented for packets "received but not processed, e.g.
due to lack of resources or unsupported protocol".
With this branch in ibmveth_poll(), a runt frame (len < ETH_HLEN) or a
frame whose offset/length overruns the pool buffer shows up in ip -s link
and /proc/net/dev as a drop, not as a length error. Each frame is still
counted once, just under a different counter.
The driver doesn't update rx_length_errors or rx_errors anywhere else in
the series either.
Would something like this work instead?
netdev->stats.rx_length_errors++;
netdev->stats.rx_errors++;
> + frames_processed++;
> + continue;
> + }
[ ... ]
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012050518.88858-1-mmc%40linux.ibm.com
