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

[Simon Horman]

On Thu, Sep 24, 2026 at 05:36:08PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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?

I've looked over the above. And while perhaps this alone doesn't warrant
a re-spin. The analysis does seem to be correct to me.