Re: [PATCH v3 net 5/7] net: enetc: pad short XDP frames coming from devmap

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

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

- [Medium] In enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the newly added short-…

--

net: enetc: pad short XDP frames coming from devmap

This adds a generic best-effort xdp_frame_pad() helper and calls it from
enetc_xdp_xmit(), together with a direct check of the head buffer length
against ENETC_MIN_BUFF_SIZE, so that short XDP frames redirected into
ENETC are padded or dropped instead of being transmitted.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index bbad942041f5a..8a9ba168eab1e 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1838,15 +1838,23 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	prefetchw(ENETC_TXBD(*tx_ring, tx_ring->next_to_use));
>  
>  	for (k = 0; k < num_frames; k++) {
> -		if (xdp_frame_has_frags(frames[k])) {
> -			shinfo = xdp_get_shared_info_from_frame(frames[k]);
> +		struct xdp_frame *xdpf = frames[k];
> +
> +		if (xdp_frame_has_frags(xdpf)) {
> +			shinfo = xdp_get_shared_info_from_frame(xdpf);
>  			if (unlikely((shinfo->nr_frags + 1) > ENETC_MAX_SKB_FRAGS))
>  				break;
>  		}
>  
> +		if (unlikely(xdp_frame_pad(xdpf) ||
> +			     xdpf->len < ENETC_MIN_BUFF_SIZE)) {
> +			tx_ring->stats.xdp_tx_drops++;
> +			break;
> +		}

[Severity: Medium]
Does this count the rejected frame twice in tx_ring->stats.xdp_tx_drops?

The new branch increments the counter and then breaks with k still pointing
at the frame that was just rejected, and the epilogue of enetc_xdp_xmit()
charges everything from k onwards again:

	tx_ring->stats.xdp_tx += k;
	tx_ring->stats.xdp_tx_drops += num_frames - k;

so the counter grows by 1 + (num_frames - k) rather than num_frames - k.

The other three break sites in this loop (the shinfo->nr_frags overflow
check, the enetc_xdp_frame_to_xdp_tx_swbd() failure and the enetc_xdp_tx()
failure) rely only on the remainder-based accounting added earlier in this
series by "net: enetc: consistenly track dropped frames in
enetc_xdp_xmit()", whose message states that the counter should be
incremented "by the remainder, not just by 1".

Would dropping the explicit increment here keep this site consistent with
the other break paths?

The branch is reachable when xdp_frame_pad() returns -EOPNOTSUPP (frame
shorter than ETH_ZLEN and fragmented) or -ENOMEM (no tailroom before the
shared info), and also when the frame total is at least ETH_ZLEN but the
head buffer is shorter than ENETC_MIN_BUFF_SIZE, which is the
XDP_REDIRECT-from-veth case described in the commit message.

Since xdp_tx_drops is exported to userspace by enetc_get_ethtool_stats()
via ethtool -S, can the reported drop count end up larger than the number
of frames the core passed to ndo_xdp_xmit()?

> +
>  		xdp_tx_bd_cnt = enetc_xdp_frame_to_xdp_tx_swbd(tx_ring,
>  							       xdp_redirect_arr,
> -							       frames[k]);
> +							       xdpf);
>  		if (unlikely(xdp_tx_bd_cnt < 0))
>  			break;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915222735.1016937-1-vladimir.oltean%40nxp.com