Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Replying to netdev's sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:

> --- Patch [1]: [PATCH net 1/2] net: macb: exclude software FCS from TX 
> byte statistics ---
> commit 24acee751440b1a44c9e16688554df0848ab621a
> Author: Nicolai Buchwitz <nb@xxxxxxxxxxx>
>
>     net: macb: exclude software FCS from TX byte statistics
>
>     Frames going through macb_pad_and_fcs() get padded and four FCS
>     bytes appended, and TX completion then accounts the grown skb->len.
>     tx_bytes is supposed to exclude the FCS, and frames padded by the
>     hardware are counted without the padding anyway, so these frames
>     show up too large in the statistics.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 76ee4f5060335..623d072a271d0 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1322,8 +1322,8 @@ static void macb_tx_error_task(struct work_struct *work)
> >  				bp->netdev->stats.tx_packets++;
> >  				queue->stats.tx_packets++;
> >  				packets++;
> > -				bp->netdev->stats.tx_bytes += skb->len;
> > -				queue->stats.tx_bytes += skb->len;
> > +				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
> > +				queue->stats.tx_bytes += tx_skb->skb_len;
> >  				bytes += skb->len;
> >  			}
> >  		} else {
>
> This is a pre-existing issue, but does macb_tx_error_task() leak BQL 
> bytes for
> dropped pending packets?
>
> When recovering from a TX error, macb_tx_error_task() iterates through 
> the TX
> ring. For pending descriptors that did not successfully transmit, the 
> code calls
> macb_tx_unmap(bp, tx_skb, 0) which frees the SKB.
>
> However, it appears to fail to accumulate the freed SKB's length into 
> the bytes
> accumulator. When netdev_tx_completed_queue() is subsequently called, 
> these
> leaked bytes are never reported to BQL, which could permanently retain 
> these
> charged bytes and stall the TX queue.

Looks like it. The dropped frames never get a completion credit, and DQL
only gets reset on close. This is independent of the statistics change,
so I would address it in a follow-up fix. Objections?

> This is another pre-existing issue, but does macb_tx_error_task() 
> modify the TX
> ring pointers without holding the appropriate lock?
>
> macb_tx_error_task() acquires bp->lock and issues a non-blocking stop 
> to the
> queues via netif_tx_stop_all_queues(). It then manipulates the ring 
> pointers
> queue->tx_head and queue->tx_tail.
>
> However, the fast-path macb_start_xmit() protects these identical 
> pointers
> using queue->tx_ptr_lock.
>
> Since netif_tx_stop_all_queues() does not wait for an already-running
> macb_start_xmit() to complete, could macb_tx_error_task() zero out the 
> ring
> pointers while macb_start_xmit() is concurrently populating descriptors 
> on
> another CPU?

AFAIU yes, stopping the queues doesn't wait for a start_xmit that's
already running. Taking tx_ptr_lock there before bp->lock, in the same
order as start_xmit, should fix that. I would handle this together with 
the
BQL issue in a follow-up series.

> [ ... ]
> > @@ -2485,6 +2487,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
> >  		return ret;
> >  	}
> >
> > +	skb_len = skb->len;
> > 	if (macb_pad_and_fcs(&skb, netdev)) {
>
> This isn't a bug introduced by this patch, but does macb_pad_and_fcs() 
> append
> an FCS before verifying TX ring space, which could corrupt the SKB upon 
> retry?
>
> [ ... ]
>
> Because the network stack will subsequently retry transmitting this 
> identical
> SKB, and its length is now larger, will macb_pad_and_fcs() compute a 
> new CRC
> over the old FCS and append a second, duplicate FCS, eventually 
> corrupting the
> payload once tailroom is exhausted?

Yes, and the copy path looks worse: the stack can requeue an skb the 
driver already
consumed. I'll do the ring space check before touching the skb at all. 
After that
point the driver never returns NETDEV_TX_BUSY, so swapping the skb 
should be fine.

> This is also a pre-existing issue, but does macb_pad_and_fcs() modify 
> shared SKB
> data for cloned packets and break nonlinear SKB layouts?
>
> If an SKB is cloned (e.g., multicast, TCP retransmissions) or nonlinear 
> (paged
> fragments), and its length is already >= ETH_ZLEN (60 bytes) so padlen 
> <= 0,
> and it has at least 4 bytes of tailroom, the code skips the 
> reallocation check:
>
> 	if (padlen <= 0) {
> 		/* FCS could be appeded to tailroom. */
> 		if (tailroom >= ETH_FCS_LEN)
> 			goto add_fcs;
>
> It then directly appends 4 bytes of FCS via skb_put_u8(). For cloned 
> SKBs, this
> modifies shared memory data. For nonlinear SKBs, skb_put() illegally 
> extends the
> linear part, which could trigger a panic in skb_panic().

The cloned part looks right. The nonlinear case IMHO can't trigger 
today:
skb_tailroom() is 0 for nonlinear skbs, so they go down 
skb_copy_expand(),
which linearizes them anyway.

I'll order the cloned check before the in-place FCS path.