Re: [net] net: macb: exclude software FCS from TX byte statistics

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

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

- [Medium] Incomplete fix: the software FCS can be appended twice to the
  same skb, so tx_bytes is still over-reported by four bytes and the new…

Pre-existing issues:
- [Critical] Use-after-free / double-free of an skb plus leak of the
  replacement skb: macb_pad_and_fcs()…
- [Medium] macb_pad_and_fcs() writes the four software FCS bytes into a
  cloned skb's shared data allocation without unsharing it.

--

net: macb: exclude software FCS from TX byte statistics

The patch records how many FCS bytes macb_pad_and_fcs() appended in
software into a new macb_tx_skb.fcs_len field, and subtracts that value
from skb->len when TX completion updates tx_bytes.  BQL accounting keeps
using the padded skb->len.

Referenced series: https://lore.kernel.org/all/20260824134703.766708-1-nb@tipi-net.de/ (local)

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
This is a pre-existing issue, not something introduced here, but can these
four skb_put_u8() writes land in data shared with another sk_buff?

macb_pad_and_fcs() computes:

	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
		      skb_is_nonlinear(*skb);

and only copies via skb_copy_expand() under "if (cloned || tailroom <
padlen)".  For a linear skb with skb->len >= ETH_ZLEN and skb_tailroom()

= ETH_FCS_LEN the earlier

	if (padlen <= 0) {
		/* FCS could be appeded to tailroom. */
		if (tailroom >= ETH_FCS_LEN)
			goto add_fcs;

jumps past that copy, so the FCS is written into the possibly shared data
area.

One case where the written bytes are still inside a clone's valid range:
packet_rcv() clones the skb for a tap, then ip_rcv_core() calls
pskb_trim_rcsum(), which only moves the original's tail.  Forwarding that
skb out a macb port then overwrites four bytes that the clone (and
userspace) still considers payload.

Would using skb_unshare()/skb_cow() before the "goto add_fcs" fast path be
appropriate here?

[Severity: Medium]
Does the requeue path still report tx_bytes four bytes too large?

macb_pad_and_fcs() runs here, before the descriptor space check later in
macb_start_xmit():

	if (CIRC_SPACE(queue->tx_head, queue->tx_tail,
		       bp->tx_ring_size) < desc_cnt) {
		netif_stop_subqueue(netdev, q);
		...
		ret = NETDEV_TX_BUSY;
		goto unlock;
	}

On NETDEV_TX_BUSY the core hands the same skb back:

sch_direct_xmit()
    if (!dev_xmit_complete(ret)) {
        /* Driver returned NETDEV_TX_BUSY - requeue skb */
        dev_requeue_skb(skb, q);

so macb_start_xmit() -> macb_pad_and_fcs() runs on that skb a second time.
The early-exit predicate (features, ip_summed, gso_size, ptp state) is
unchanged, padlen = ETH_ZLEN - skb->len is now <= 0, so the code reaches
add_fcs again and appends a second four-byte CRC computed over
payload+FCS1.

macb_tx_map() then stores fcs_len = ETH_FCS_LEN while skb->len contains
2 * ETH_FCS_LEN software bytes, so macb_tx_complete() and
macb_tx_error_task() still add four bytes too many:

	bp->netdev->stats.tx_bytes += skb->len - tx_skb->fcs_len;

and the frame on the wire carries the stale first FCS as trailing payload.

Reaching the NETDEV_TX_BUSY branch needs desc_cnt >= 2, i.e. a
software-FCS frame longer than bp->max_tx_length.  In-tree configurations
allow that: mpfs_config has .max_tx_length = 4040 with .jumbo_max_len =
4040, and pic64hpsc_config has .jumbo_max_len = 16383 against
GEM_MAX_TX_LEN of 16320, so with jumbo MTU a frame grown by the software
FCS spans two descriptors and CIRC_SPACE == 1 yields NETDEV_TX_BUSY.

Would moving the macb_pad_and_fcs() call after the CIRC_SPACE check (or
otherwise making the append idempotent) address this?  Since the chosen
representation records only the length of the last append, should the
changelog also mention the requeue interaction?

[Severity: Critical]
This isn't a bug introduced by this patch, but the same ordering looks like
it can free the caller's skb and then tell the core to reuse it.

macb_pad_and_fcs() substitutes the skb only through the caller's local
pointer:

	if (cloned || tailroom < padlen) {
		nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC);
		if (!nskb)
			return -ENOMEM;

		dev_consume_skb_any(*skb);
		*skb = nskb;
	}

If macb_start_xmit() afterwards takes the

		ret = NETDEV_TX_BUSY;
		goto unlock;

path, nskb is dropped on the floor and the core continues with the
original pointer:

dev_hard_start_xmit()
    rc = xmit_one(skb, dev, txq, next != NULL);
    if (unlikely(!dev_xmit_complete(rc))) {
            skb->next = next;
            goto out;

That is a write into the already freed sk_buff, and sch_direct_xmit() then
calls dev_requeue_skb() on it, so it is dequeued, transmitted and freed a
second time, while nskb leaks.

The preconditions coexist: a forwarded or bridged nonlinear frame has
skb_tailroom() == 0 and cloned == true, so the skb_copy_expand() /
dev_consume_skb_any() path is always taken, and on mpfs_config
(.max_tx_length = 4040, .jumbo_max_len = 4040) or pic64hpsc_config
(GEM_MAX_TX_LEN 16320 against .jumbo_max_len 16383) the frame plus the
four FCS bytes needs two descriptors, so CIRC_SPACE == 1 returns
NETDEV_TX_BUSY.

Should the space check happen before any skb is consumed or replaced?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831113128.1678674-1-nb%40tipi-net.de