netdev - Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220208160742.GB4785@euler>
Date:   Tue, 8 Feb 2022 08:07:42 -0800
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for
 stats

On Tue, Feb 08, 2022 at 03:34:49PM +0000, Vladimir Oltean wrote:
> On Tue, Feb 08, 2022 at 05:03:03PM +0200, Vladimir Oltean wrote:
> > >  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> > > +		unsigned int idx = 0;
> > > +
> > 
> > This is a bug which causes ocelot->stats to be overwritten with the
> > statistics of port 0, for all ports. Either move the variable
> > declaration and initialization with 0 in the larger scope (outside the
> > "for" loop), or initialize idx with i * ocelot->num_stats.
> 
> My analysis was slightly incorrect. Somehow I managed to fool myself
> into thinking that you had tested this in a limited scenario, hence the
> reason you didn't notice it's not working. But apparently you didn't
> test with traffic at all.
> 
> So ocelot->stats isn't overwritten with the stats of port 0 for all
> ports. But rather, all ports write into the ocelot->stats space
> dedicated for port 0, effectively overwriting the stats of port 0 with
> the stats of the last port. And no one populates the ocelot->stats space
> for ports [1 .. last]. So no port has good statistics, I don't see a
> circumstance where testing could have misled you.

Both ethtool -S and debugfs show statistics that I'd expect in my test
setup. But yes, the port 0 stats would be especially curious.

EDIT: Just saw your message about these. Yes, port 0 is all messed up:
# ethtool -S eth0 | grep p00 | head
     p00_rx_octets: 13602161426432
     p00_rx_unicast: 4539780431872
     p00_rx_multicast: 9028021256192
     p00_rx_broadcast: 8980776615936
     p00_rx_shorts: 0
     p00_rx_fragments: 0
     p00_rx_jabbers: 0
     p00_rx_crc_align_errs: 0
     p00_rx_sym_errs: 0
     p00_rx_frames_below_65_octets: 4539780431872


(configuration - swp2 plugged into swp3, bridged with stp. swp1 into my
development machine, swp4-7 not up)

# pwd
/sys/class/net
# cat swp[123]/statistics/tx_bytes
44616
44668
52
# cat swp[123]/statistics/rx_bytes
34574
46
73272

# ethtool -S swp1 | head -n 5
NIC statistics:
     tx_packets: 942
     tx_bytes: 48984
     rx_packets: 764
     rx_bytes: 37808
# ethtool -S swp2 | head -n 5
NIC statistics:
     tx_packets: 946
     tx_bytes: 49192
     rx_packets: 1
     rx_bytes: 46
# ethtool -S swp3 | head -n 5
NIC statistics:
     tx_packets: 1
     tx_bytes: 52
     rx_packets: 1699
     rx_bytes: 80658
# ethtool -S swp4 | head -n 5
NIC statistics:
     tx_packets: 0
     tx_bytes: 0
     rx_packets: 0
     rx_bytes: 0
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1520 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
3: swp1@...0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
4: swp2@...0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
5: swp3@...0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
6: swp4@...0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
7: swp5@...0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
8: swp6@...0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
9: swp7@...0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
10: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff


Clearly my testing wasn't sufficient as there were still issues, but
there were reasonable signs of things working as expected on ports 1-4.