Re: [PATCH net v2 1/3] can: dev: prevent bus-off recovery when the interface is not running

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 (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).

- [Medium] Split-frame drop accounting in stmmac_rx_zc() is inconsistent
  per frame and per core.

Pre-existing issues:
- [High] stmmac_dispatch_skb_zc() (stmmac_main.c:5533) sets
  `skb->ip_summed = CHECKSUM_UNNECESSARY` for every IP frame when RX COE…
- [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
  wide unsigned longs.

--

net: stmmac: drop split frames on AF_XDP zero-copy RX

This patch fixes stmmac_rx_zc() so that split frames are drained as
errored, and the last fragment is no longer handed to XDP as a fresh
frame. It also changes the legacy normal and enhanced descriptor
callbacks to report rx_not_ls together with discard_frame for non-last
descriptors.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 0512536012251..297e2428a8f9b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}
>  
>  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> index c4b613564f87c..d19bf7ca4db38 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> @@ -70,7 +70,7 @@ static int ndesc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}
>  
>  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f0835be2f65a7..10ea72cbf889a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats,
>  						  rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> -			xsk_buff_free(buf->xdp);
> -			buf->xdp = NULL;
> -			dirty++;
> +
> +		if (unlikely(status & discard_frame)) {
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}
>  
> -		if (unlikely(error && (status & rx_not_ls)))
> -			goto read_again;
>  		if (unlikely(error)) {
> +			xsk_buff_free(buf->xdp);
> +			buf->xdp = NULL;
> +			dirty++;
> +
> +			if (status & rx_not_ls)
> +				goto read_again;
> +
>  			count++;
>  			continue;
>  		}
> @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (likely(status & rx_not_ls)) {
>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> +			rx_dropped++;
> +			error = 1;

[Severity: Medium]

Is the split-frame drop accounting here consistent across descriptor
formats?

The legacy callbacks now return discard_frame | rx_not_ls for every
non-last descriptor. So the discard branch above runs first for each of
those descriptors, and there is no !error guard:

    if (unlikely(status & discard_frame)) {
        error = 1;
        if (!priv->hwts_rx_en)
            rx_errors++;
    }

On dwmac100/dwmac1000, a frame spanning N descriptors would then add N-1
to rx_errors. It adds 0 to rx_dropped, while count / rx_pkt_n goes up
only once for the frame. The new rx_dropped++ in this 1:1 block can never
be reached on those cores.

On dwmac4 (and similarly xgmac), dwmac4_wrback_get_rx_status() returns a
bare rx_not_ls for non-last descriptors:

	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
		return rx_not_ls;

That means this block does rx_dropped++ and sets error = 1. If the last
descriptor has RDES3_ERROR_SUMMARY set, the discard branch also does
rx_errors++. The same dropped frame then shows up in both rx_dropped and
rx_errors.

Later in the series, stmmac_rx() is changed to count only once, using
(!error && (status & discard_frame)). Should stmmac_rx_zc() get the same
treatment, so each dropped frame is counted once in either rx_errors or
rx_dropped?

>  			dirty++;
> -			count++;
>  			goto read_again;
>  		}
>  

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The
new rx_dropped++ feeds into an unsynchronized accumulation at the end of
stmmac_rx_zc():

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

These are device-wide unsigned longs. They are updated with a plain +=
from per-queue NAPI contexts that can run at the same time on different
CPUs. stmmac_rx() does the same thing, and stmmac_get_stats64() reads
them without any locking.

Can concurrent NAPI polls on different queues lose increments to these
counters?

[Severity: High]

This isn't a bug introduced by this patch, but it is in the same receive
path. On XDP_PASS, stmmac_rx_zc() calls stmmac_dispatch_skb_zc() without
passing status. That function marks every IP frame as verified when RX
COE is enabled:

stmmac_dispatch_skb_zc() {
    ...
	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
		skb_checksum_none_assert(skb);
	else
		skb->ip_summed = CHECKSUM_UNNECESSARY;
    ...
}

On dwmac4, dwmac4_wrback_get_rx_status() reports L4 payload checksum
failures only as csum_none, without discard_frame:

	if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
		x->ip_payload_err++;
		ret |= csum_none;
	}

Also, dwmac4_dma_rx_chan_op_mode() sets MTL_OP_MODE_DIS_TCP_EF, so the
hardware forwards these frames instead of dropping them.

Suppose a frame with a bad TCP/UDP checksum arrives on a ZC queue and
the XDP program passes it. Could it reach the stack as
CHECKSUM_UNNECESSARY and hand corrupted payload to applications?

The non-ZC stmmac_rx() handles this case:

	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb) ||
	    (status & csum_none))
		skb_checksum_none_assert(skb);

This still seems to be the case at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_196A2433801631CD2FA42998465E26316105%40qq.com