Re: [PATCH net 1/6] net: bcmasp: fix mib counters struct alignment with ethtool stats

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] The commit message overstates what the bug did.

--

net: bcmasp: fix mib counters struct alignment with ethtool stats

This commit removes the leftover edpkt_* fields from the start of struct
bcmasp_mib_counters. They were left in place when the matching
bcmasp_gstrings_stats[] entries were removed for asp-v3.0. Since then the
struct layout has not matched the order the ethtool code indexes by.

> Because bcmasp_update_mib_counters() and bcmasp_get_ethtool_stats() index
> into struct bcmasp_mib_counters sequentially based on the order of
> bcmasp_gstrings_stats, this 16-byte offset caused hardware statistics
> to be written to the wrong struct members and software counters to be
> read from incorrect offsets (and reading out-of-bounds at the end of the
> struct).

[Severity: Medium]
Is the out-of-bounds part of this description accurate?

Before this patch, struct bcmasp_mib_counters had 17 u32 members (68
bytes), and bcmasp_gstrings_stats[] has 13 entries. Both loops stop at
BCMASP_STATS_LEN:

drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:bcmasp_get_ethtool_stats() {
    ...
	for (i = 0; i < BCMASP_STATS_LEN; i++) {
		p = (char *)(&intf->mib) + (i * sizeof(u32));
		data[i] = *(u32 *)p;
	}
}

So the highest offset either loop touches is 12 * 4 = 48 bytes, which is
inside the 68-byte struct.

The hardware values also appear to have reached userspace correctly.
bcmasp_update_mib_counters() writes them to raw slots 0-4, and
bcmasp_get_ethtool_stats() reads them back from the same slots. The only
members overwritten were edpkt_* and umac_frm_cnt, and no code uses those
by name.

The part that looks broken is the software counters. Code increments them
by name (intf->mib.alloc_rx_skb_failed++, tx_dma_failed++, and so on).
They sit in raw slots 9-16, but ethtool read slots 5-12. As a result:

  - "RX SKB Alloc Failed", "TX DMA Failed", "Multicast Filters Full" and
    "Unicast Filters Full" read fb_* members that are never written, so
    they always showed 0.

  - The next four strings showed the values of the first four software
    counters.

  - filters_combine_cnt, promisc_filters_cnt, tx_realloc_offload_failed
    and tx_timeout_cnt were never shown at all.

Could the commit message describe this as shifted or mislabeled software
counters, rather than as an out-of-bounds read at the end of the struct?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com