Replying to netdev's sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:
quoted hunk ↗ jump to hunk
--- Patch [1]: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics ---
commit 24acee751440b1a44c9e16688554df0848ab621a
Author: Nicolai Buchwitz [off-list ref]
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.
quoted
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.
[ ... ]
quoted
@@ -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.