linux-kernel - [PATCH] x86/percpu: Avoid comparing unsigned types to -1

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>] [day] [month] [year] [list]

Message-Id: <20241025191041.40169-1-dave.hansen@linux.intel.com>
Date: Fri, 25 Oct 2024 12:10:41 -0700
From: Dave Hansen <dave.hansen@...ux.intel.com>
To: dave.hansen@...el.com
Cc: x86@...nel.org,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Tejun Heo <tj@...nel.org>,
	Christoph Lameter <cl@...ux.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: [PATCH] x86/percpu: Avoid comparing unsigned types to -1

clang warns when comparing an unsinged type to -1 since the comparison
is always false.

This can be quickly reproduced by setting CONFIG_WERROR=y and running:

	make W=1 CC=clang-14 net/ipv4/tcp_output.o

net/ipv4/tcp_output.c:187:3: error: result of comparison of constant -1 with expression of type 'u8' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
  187 |                 NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  188 |                               tp->compressed_ack);
      |                               ~~~~~~~~~~~~~~~~~~~
...
arch/x86/include/asm/percpu.h:238:31: note: expanded from macro 'percpu_add_op'
  238 |                               ((val) == 1 || (val) == -1)) ?            \
      |                                              ~~~~~ ^  ~~

Fix this by avoiding a comparison of an uncast -1 to 'val'.

Doing this in addition to the existing 'pao_ID__' calculation would make it
even more unreadable. Remove 'pao_ID__' and replace it with the three
components of its calculation.

This preserves some unintuitive but useful behavior.  For instance, gcc sees:

	percpu_add_op(..., var, (u8)-1);

and can transform that into a "dec".  Clang, on the other hand, sees the 'u8'
type and assumes that "(val) == -1" is false, which was the root of the
warning.

This is useful gcc behavior because:

	#define this_cpu_sub(pcp, val)  this_cpu_add(pcp, -(typeof(pcp))(val))

so any code that does:

	this_cpu_sub(A, 1)

where 'A' is an unsigned type generates a "dec".  Clang, on the other
hand generates a less-efficient "add".

Reported-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Signed-off-by: Dave Hansen <dave.hansen@...ux.intel.com>
---
 arch/x86/include/asm/percpu.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index c55a79d5feae..57d9759c692e 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -234,18 +234,19 @@ do {									\
  */
 #define percpu_add_op(size, qual, var, val)				\
 do {									\
-	const int pao_ID__ = (__builtin_constant_p(val) &&		\
-			      ((val) == 1 || (val) == -1)) ?		\
-				(int)(val) : 0;				\
+	const int pao_const__ = __builtin_constant_p(val);		\
+	const int pao_inc__   = (val) == 1;				\
+	const int pao_dec__   = (typeof(var))(val) ==			\
+				(typeof(var))-1;			\
 									\
 	if (0) {							\
 		typeof(var) pao_tmp__;					\
 		pao_tmp__ = (val);					\
 		(void)pao_tmp__;					\
 	}								\
-	if (pao_ID__ == 1)						\
+	if (pao_const__ && pao_inc__)					\
 		percpu_unary_op(size, qual, "inc", var);		\
-	else if (pao_ID__ == -1)					\
+	else if (pao_const__ && pao_dec__)				\
 		percpu_unary_op(size, qual, "dec", var);		\
 	else								\
 		percpu_binary_op(size, qual, "add", var, val);		\
-- 
2.34.1