Message-ID: <a30fec0bde9c4ab3a195704653aae3c1@AcuMS.aculab.com>
Date:   Thu, 22 Dec 2022 22:33:49 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     David Laight <David.Laight@...LAB.COM>,
        'Linus Torvalds' <torvalds@...ux-foundation.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Joe Perches" <joe@...ches.com>
Subject: Subject: [RFC PATCH v2 1/1] Slightly relax the type checking done by
 min() and max().

Slightly relax the type checking done by min() and max().

Relax the type checking done by min() and max() to:
- Allow comparisons of any two signed values.
- Allow comparisons of any two unsigned values.
- Allow comparisons where an unsigned value is promoted to a signed type.
  Typically u8 and u16 variables, but also u32 against s64.
- Allow comparisons of unsigned values against non-negative signed constants.
  So min(unsigned_var, 5) and min(unsigned_var, 5u) are both allowed.

Comparisons of signed values against 31-bit unsigned constants are disallowed.
In particular min(signed_var, sizeof(...)) is disallowed.

Two new pairs of functions are added:
min_signed() and max_signed()
  These are identical to min() and max() except that signed values
  can be compared against 31-bit unsigned constants.
  So min_signed(signed_var, sizeof(...)) is allowed and can be negative.

min_unsigned() and max_unsigned()
  These promote their arguments to unsigned types before the comparison.
  They should be used when a signed expression is known to be positive.
  Unlike min_t(unsigned_type, x, y) high bits of x and y will not be
  discarded if the wrong 'unsigned_type' is picked.

These reduce the need to use min_t/max_t() and the possibly unwanted
  side effects if a type that is too small is specified.
It isn't hard to find places where 'a = min(b, c)' has been replaced
  with 'a = min_t(typeof(a), b, c)' due to a type mismatch between b and c.
  But the correct type is an unsigned type not smaller than b or c.

Changes for v2:
- factor out compile type check using:
  +#define __typed_null(type, value) (1 ? ((void *)((long)(value))) : (type *)0)
- Disallow min(signed_var, 32u) (Linus didn't like it).
- Allow for 32bit unsigned values being promoted to 64bit signed ones.
- Use explicit tests rather than a comparison on the pointer types.
- Bug fixes

Posted as an RFC because it doesn't really make sense to have min_unsigned()
since min_signed() will pretty much always have the same effect and the
extra function just adds confusion.
But I still think that min(signed_var, 16u) should be valid so it could be
squashed out the other way.
---
 include/linux/minmax.h | 127 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 106 insertions(+), 21 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 5433c08fcc68..1351f1b7f9a8 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -9,47 +9,132 @@
  *
  * - avoid multiple evaluations of the arguments (so side-effects like
  *   "x++" happen only once) when non-constant.
- * - perform strict type-checking (to generate warnings instead of
- *   nasty runtime surprises). See the "unnecessary" pointer comparison
- *   in __typecheck().
+ * - perform type-checking (to generate warnings instead of nasty runtime
+ *   surprises).
  * - retain result as a constant expressions when called with only
  *   constant expressions (to avoid tripping VLA warnings in stack
  *   allocation usage).
+ *
+ * The type-check normally allows:
+ * - comparison of two signed expressions.
+ * - comparison of two unsigned expressions.
+ * - comparison of unsigned char and short variables to a signed expression.
+ * - comparison of unsigned expresions to 32bit non-negative signed constants.
+ * The last allows min(unsigned_var, 16) as well as min(unsigned_var, 16u);
+ * Other comparisons between signed and unsigned values generate a compile
+ * time error.
+ * Three additional function pairs futher relax the check:
+ * - min_signed() allows signed expressions against 31bit unsigned constants.
+ *   So min_signed(foo, sizeof (bar)) is valid - but might be negative.
+ * - min_unsigned() converts both values to an unsigned type.
+ *   So max_unsigned(foo, sizeof (bar)) is valid but can be very large
+ *   (effectively undefined) if foo is negative.
+ * - min_t(type, a, b) casts both values to (type) BEFORE the comparison.
+ *   misuse can easily cause truncated values.
+ */
+
+/*
+ * Compile time test for compile time constant zero.
+ * The type of the result depends on the value.
+ * Similar to: (value ? (void *)value : (type *)0)
  */
-#define __typecheck(x, y) \
-	(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
+#define __typed_null(type, value) (1 ? ((void *)((long)(value))) : (type *)0)
+
+/* True if (long)x is zero */
+#define __is_constzero(x) (sizeof(*__typed_null(int, x)) == sizeof(int))
+
+/* Compile time 'type error' if cond is zero */
+#define __type_error(cond) \
+	((int)sizeof(__typed_null(int, cond) == (long *)0) * 0)
+
+/* True if x (any type) is constant in the range [0..0x7fffffff] */
+#define __is_positive(x) \
+	 __is_constzero((x) != (typeof(x))((long)(x) & 0x7fffffffu))
+
+/* True if x has a signed type */
+#define __is_signed(x) is_signed_type(typeof(x))
 
-#define __no_side_effects(x, y) \
-		(__is_constexpr(x) && __is_constexpr(y))
+/* True if x will be promoted to a larger signed type */
+#define __is_small(x, y) (sizeof(x) < sizeof((y) + 0u))
 
-#define __safe_cmp(x, y) \
-		(__typecheck(x, y) && __no_side_effects(x, y))
+/* Allow comparisons that can't promote negative signed to unsigned. */
+#define __types_compatible(x, y) \
+	(__is_signed(x) ? __is_positive(x) || __is_small(y, x) || __is_signed(y) \
+			: __is_positive(y) || __is_small(x, y) || !__is_signed(y))
+
+/* Compile error if the types don't match, value is a constant 0. */
+#define __typecheck(__allow_const, x, y) \
+	__type_error(__types_compatible(x, y) || (__allow_const && \
+		(__is_positive(x) || __is_positive(y))))
+
+/* Cast positive constants to int, they may get promoted back to unsigned. */
+#define __maybe_int_cast(x) \
+	__builtin_choose_expr(__is_positive(x), (int)(long)(x), x)
 
 #define __cmp(x, y, op)	((x) op (y) ? (x) : (y))
 
 #define __cmp_once(x, y, unique_x, unique_y, op) ({	\
-		typeof(x) unique_x = (x);		\
-		typeof(y) unique_y = (y);		\
-		__cmp(unique_x, unique_y, op); })
+	typeof(__maybe_int_cast(x)) unique_x = (x);	\
+	typeof(__maybe_int_cast(y)) unique_y = (y);	\
+	__cmp(unique_x, unique_y, op); })
+
+#define __careful_cmp(__allow_const, x, y, op)				\
+	(__typecheck(__allow_const, x, y) +				\
+	 __builtin_choose_expr(__is_constexpr(x) && __is_constexpr(y),	\
+		__cmp(__maybe_int_cast(x), __maybe_int_cast(y), op),	\
+		__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op)))
+
+/**
+ * min - return minimum of two values of the same or compatible types.
+ *       Unsigned values may be compared against positive signed constants.
+ *       min(unsigned_expr, 5) is valid but min(signed_expr, 5u) is not.
+ * @x: first value
+ * @y: second value
+ */
+#define min(x, y)	__careful_cmp(0, x, y, <)
 
-#define __careful_cmp(x, y, op) \
-	__builtin_choose_expr(__safe_cmp(x, y), \
-		__cmp(x, y, op), \
-		__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
+/**
+ * min_signed - return minimum of two values of the same or compatible types.
+ *       Values may be compared against positive constants.
+ *       Unlike min(), min_signed(signed_expr, 5u) is valid and may be negative.
+ * @x: first value
+ * @y: second value
+ */
+#define min_signed(x, y)	__careful_cmp(1, x, y, <)
 
 /**
- * min - return minimum of two values of the same or compatible types
+ * min_unsigned - return minimum of two values converted to an unsigned type.
+ *       Negative values will be treated as large positive values.
  * @x: first value
  * @y: second value
  */
-#define min(x, y)	__careful_cmp(x, y, <)
+#define min_unsigned(x, y)	min((x) + 0u + 0ull, (y) + 0u + 0ull)
 
 /**
  * max - return maximum of two values of the same or compatible types
+ *       Unsigned values may be compared against positive signed constants.
+ *       max(unsigned_expr, 5) is valid but max(signed_expr, 5u) is not.
+ * @x: first value
+ * @y: second value
+ */
+#define max(x, y)	__careful_cmp(0, x, y, >)
+
+/**
+ * max_signed - return maximum of two values of the same or compatible types
+ *       Values may be compared against positive constants.
+ *       Unlike max(), max_signed(signed_expr, 5u) is valid.
+ * @x: first value
+ * @y: second value
+ */
+#define max_signed(x, y)	__careful_cmp(1, x, y, >)
+
+/**
+ * max_unsigned - return maximum of two values converted to an unsigned type.
+ *       Negative values will be treated as large positive values.
  * @x: first value
  * @y: second value
  */
-#define max(x, y)	__careful_cmp(x, y, >)
+#define max_unsigned(x, y)	max((x) + 0u + 0ull, (y) + 0u + 0ull)
 
 /**
  * min3 - return minimum of three values
@@ -101,7 +186,7 @@
  * @x: first value
  * @y: second value
  */
-#define min_t(type, x, y)	__careful_cmp((type)(x), (type)(y), <)
+#define min_t(type, x, y)	__careful_cmp(0, (type)(x), (type)(y), <)
 
 /**
  * max_t - return maximum of two values, using the specified type
@@ -109,7 +194,7 @@
  * @x: first value
  * @y: second value
  */
-#define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
+#define max_t(type, x, y)	__careful_cmp(0, (type)(x), (type)(y), >)
 
 /**
  * clamp_t - return a value clamped to a given range using a given type
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists