netdev - Re: [PATCH net] net/smc: Fix pos miscalculation in statistics

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]

Date: Wed, 11 Oct 2023 11:14:22 +0800
From: Dust Li <dust.li@...ux.alibaba.com>
To: Wenjia Zhang <wenjia@...ux.ibm.com>, David Miller <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, linux-s390@...r.kernel.org,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Heiko Carstens <hca@...ux.ibm.com>,
	Jan Karcher <jaka@...ux.ibm.com>,
	Alexandra Winter <wintera@...ux.ibm.com>,
	Karsten Graul <kgraul@...ux.ibm.com>,
	Stefan Raspl <raspl@...ux.ibm.com>,
	Gerd Bayer <gbayer@...ux.ibm.com>,
	Thorsten Winkler <twinkler@...ux.ibm.com>,
	Halil Pasic <pasic@...ux.ibm.com>,
	Nils Hoppmann <niho@...ux.ibm.com>,
	Niklas Schnell <schnelle@...ux.ibm.com>,
	Tony Lu <tonylu@...ux.alibaba.com>,
	Wen Gu <guwen@...ux.alibaba.com>,
	"D. Wythe" <alibuda@...ux.alibaba.com>
Subject: Re: [PATCH net] net/smc: Fix pos miscalculation in statistics

On Mon, Oct 09, 2023 at 04:40:48PM +0200, Wenjia Zhang wrote:
>From: Nils Hoppmann <niho@...ux.ibm.com>
>
>SMC_STAT_PAYLOAD_SUB(_smc_stats, _tech, key, _len, _rc) will calculate
>wrong bucket positions for payloads of exactly 4096 bytes and
>(1 << (m + 12)) bytes, with m == SMC_BUF_MAX - 1.
>
>Intended bucket distribution:
>Assume l == size of payload, m == SMC_BUF_MAX - 1.
>
>Bucket 0                : 0 < l <= 2^13
>Bucket n, 1 <= n <= m-1 : 2^(n+12) < l <= 2^(n+13)
>Bucket m                : l > 2^(m+12)
>
>Current solution:
>_pos = fls64((l) >> 13)
>[...]
>_pos = (_pos < m) ? ((l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m
>
>For l == 4096, _pos == -1, but should be _pos == 0.
>For l == (1 << (m + 12)), _pos == m, but should be _pos == m - 1.
>
>In order to avoid special treatment of these corner cases, the
>calculation is adjusted. The new solution first subtracts the length by
>one, and then calculates the correct bucket by shifting accordingly,
>i.e. _pos = fls64((l - 1) >> 13), l > 0.
>This not only fixes the issues named above, but also makes the whole
>bucket assignment easier to follow.
>
>Same is done for SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _len),
>where the calculation of the bucket position is similar to the one
>named above.
>
>Fixes: e0e4b8fa5338 ("net/smc: Add SMC statistics support")
>Suggested-by: Halil Pasic <pasic@...ux.ibm.com>
>Signed-off-by: Nils Hoppmann <niho@...ux.ibm.com>
>Reviewed-by: Halil Pasic <pasic@...ux.ibm.com>
>Reviewed-by: Wenjia Zhang <wenjia@...ux.ibm.com>


Good catch, thanks !

Reviewed-by: Dust Li <dust.li@...ux.alibaba.com>

>---
> net/smc/smc_stats.h | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
>index aa8928975cc6..9d32058db2b5 100644
>--- a/net/smc/smc_stats.h
>+++ b/net/smc/smc_stats.h
>@@ -92,13 +92,14 @@ do { \
> 	typeof(_smc_stats) stats = (_smc_stats); \
> 	typeof(_tech) t = (_tech); \
> 	typeof(_len) l = (_len); \
>-	int _pos = fls64((l) >> 13); \
>+	int _pos; \
> 	typeof(_rc) r = (_rc); \
> 	int m = SMC_BUF_MAX - 1; \
> 	this_cpu_inc((*stats).smc[t].key ## _cnt); \
>-	if (r <= 0) \
>+	if (r <= 0 || l <= 0) \
> 		break; \
>-	_pos = (_pos < m) ? ((l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m; \
>+	_pos = fls64((l - 1) >> 13); \
>+	_pos = (_pos <= m) ? _pos : m; \
> 	this_cpu_inc((*stats).smc[t].key ## _pd.buf[_pos]); \
> 	this_cpu_add((*stats).smc[t].key ## _bytes, r); \
> } \
>@@ -138,9 +139,12 @@ while (0)
> do { \
> 	typeof(_len) _l = (_len); \
> 	typeof(_tech) t = (_tech); \
>-	int _pos = fls((_l) >> 13); \
>+	int _pos; \
> 	int m = SMC_BUF_MAX - 1; \
>-	_pos = (_pos < m) ? ((_l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m; \
>+	if (_l <= 0) \
>+		break; \
>+	_pos = fls((_l - 1) >> 13); \
>+	_pos = (_pos <= m) ? _pos : m; \
> 	this_cpu_inc((*(_smc_stats)).smc[t].k ## _rmbsize.buf[_pos]); \
> } \
> while (0)
>-- 
>2.40.1