linux-kernel - RE: [PATCH net-next 2/3] net: enetc: separate 64-bit counters from enetc_port

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:
 <AM9PR04MB850500D3FC24FE23DEFCEA158879A@AM9PR04MB8505.eurprd04.prod.outlook.com>
Date: Mon, 23 Jun 2025 03:13:16 +0000
From: Wei Fang <wei.fang@....com>
To: Simon Horman <horms@...nel.org>
CC: Claudiu Manoil <claudiu.manoil@....com>, Vladimir Oltean
	<vladimir.oltean@....com>, Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: RE: [PATCH net-next 2/3] net: enetc: separate 64-bit counters from
 enetc_port_counters

> This patch looks fine to me, as does the following one.
> However, they multiple sparse warnings relating
> to endianness handling in the ioread32() version of _enetc_rd_reg64().
> 
> I've collected together my thoughts on that in the form of a patch.
> And I'd appreciate it if we could resolve this one way or another.
> 
> From: Simon Horman <horms@...nel.org>
> Subject: [PATCH RFC net] net: enetc: Correct endianness handling in
>  _enetc_rd_reg64
> 
> enetc_hw.h provides two versions of _enetc_rd_reg64.
> One which simply calls ioread64() when available.
> And another that composes the 64-bit result from ioread32() calls.
> 
> In the second case the code appears to assume that each ioread32()
> call returns a little-endian value. The high and the low 32 bit
> values are then combined to make a 64-bit value which is then
> converted to host byte order.
> 
> However, both the bit shift and the logical or used to combine
> the two 32-bit values assume that they are operating on host-byte
> order entities. This seems broken and I assume that the code
> has only been tested on little endian systems.
> 
> Correct this by converting the 32-bit little endian values
> to host byte order before operating on them.
> 
> Also, use little endian types to store these values, to make
> the logic clearer and is moreover good practice.
> 
> Flagged by Sparse
> 
> Fixes: 69c663660b06 ("net: enetc: Correct endianness handling in
> _enetc_rd_reg64")

I think the fixes tag should be:
Fixes: 16eb4c85c964 ("enetc: Add ethtool statistics")

> Signed-off-by: Simon Horman <horms@...nel.org>
> ---
> I have marked this as RFC as I am unsure that the above is correct.
> 
> The version of _enetc_rd_reg64() that is a trivial wrapper around
> ioread64() assumes that the call to ioread64() returns a host byte order
> value?

Yes, ioread64() returns a host endian value, below is the definition
of ioread64() in include/asm-generic/io.h.

static inline u64 ioread64(const volatile void __iomem *addr)
{
	return readq(addr);
}

static inline u64 readq(const volatile void __iomem *addr)
{
	u64 val;

	log_read_mmio(64, addr, _THIS_IP_, _RET_IP_);
	__io_br();
	val = __le64_to_cpu((__le64 __force)__raw_readq(addr));
	__io_ar(val);
	log_post_read_mmio(val, 64, addr, _THIS_IP_, _RET_IP_);
	return val;
}

And ioread32() is also defined similarly, so ioread32() also returns a
host endian value.

static inline u32 ioread32(const volatile void __iomem *addr)
{
	return readl(addr);
}

static inline u32 readl(const volatile void __iomem *addr)
{
	u32 val;

	log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
	__io_br();
	val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
	__io_ar(val);
	log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
	return val;
}
> 
> If that is the case then is it also the case that the ioread32() calls,
> in this version of _enetc_rd_reg64() also return host byte order values.
> And if so, it is probably sufficient for this version to keep using u32
> as the type for low, high, and tmp.  And simply:
> 
> 	return high << 32 | low;

Yes, this change is enough. BTW, currently, the platforms using ENETC
are all arm64, so ioread64() is used to read registers. Therefore, it does
not cause any problems in actual use. However, from the driver's
perspective, it should indeed be fixed. Thanks very much.