netdev - Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

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: <87fsccvy2o.fsf@kurt>
Date:   Sun, 15 Jan 2023 12:25:51 +0100
From:   Kurt Kanzenbach <kurt@...utronix.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org,
        Vijayakannan Ayyathurai <vijayakannan.ayyathurai@...el.com>,
        Wong Vee Khee <vee.khee.wong@...ux.intel.com>
Subject: Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

Hi Andrew,

On Sat Jan 14 2023, Andrew Lunn wrote:
> On Sat, Jan 14, 2023 at 01:04:37PM +0100, Kurt Kanzenbach wrote:
>> Correct queue statistics reading. All queue statistics are stored as unsigned
>> long values. The retrieval for ethtool fetches these values as u64. However, on
>> some systems the size of the counters are 32 bit.
>
>> @@ -551,16 +551,16 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
>>  		p = (char *)priv + offsetof(struct stmmac_priv,
>>  					    xstats.txq_stats[q].tx_pkt_n);
>>  		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
>> -			*data++ = (*(u64 *)p);
>> -			p += sizeof(u64 *);
>> +			*data++ = (*(unsigned long *)p);
>> +			p += sizeof(unsigned long);
>
> As you said in the comment, the register is 32 bits.

Maybe the commit message is a bit misleading. There are no registers
involved here. The queue statistics are accounted in software. See
stmmac_txq_stats and stmmac_rxq_stats.

> So maybe u32 would be better than unsigned long?

The problem is pkt_n and irq_n are stored as longs. The size of these
are either 4 or 8 byte depending on the architecture this code runs
on. I my opinion we cannot use u32 or u64 for that reason.

BTW, all the other stmmac statistic counters follow a different
pattern. For example:

for (i = 0; i < STMMAC_MMC_STATS_LEN; i++) {
	char *p;
	p = (char *)priv + stmmac_mmc[i].stat_offset;

	data[j++] = (stmmac_mmc[i].sizeof_stat ==
		     sizeof(u64)) ? (*(u64 *)p) :
		     (*(u32 *)p);
}

> And it would also avoid issues if this code is every used on a 64 bit
> machine.

The current code runs fine on 64 bit architectures such as Intel EHL,
TGL, ADL, ... I'm trying to fix it for the 32 bit case.

Thanks,
Kurt

Download attachment "signature.asc" of type "application/pgp-signature" (862 bytes)