Re: [PATCH v10 3/9] compiler_types.h: Add assert_type to catch type mis-match while compiling

From: Gwan-gyeong Mun
Date: Wed Sep 21 2022 - 10:11:28 EST

Next message: Vladimir Oltean: "Re: [PATCH net-next 08/13] net: dsa: hellcreek: deny tc-taprio changes to per-tc max SDU"
Previous message: Liang, Kan: "Re: [PATCH] KVM: x86/pmu: Add PEBS support for Intel Sapphire Rapids"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 9/13/22 3:01 PM, Kees Cook wrote:

On Fri, Sep 09, 2022 at 07:59:07PM +0900, Gwan-gyeong Mun wrote:

It adds assert_type and assert_typable macros to catch type mis-match while
compiling. The existing typecheck() macro outputs build warnings, but the
newly added assert_type() macro uses the _Static_assert() keyword (which is
introduced in C11) to generate a build break when the types are different
and can be used to detect explicit build errors.
Unlike the assert_type() macro, assert_typable() macro allows a constant
value as the second argument.

Suggested-by: Kees Cook <keescook@xxxxxxxxxxxx>
Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Nirmoy Das <nirmoy.das@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>
---
include/linux/compiler_types.h | 39 ++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 4f2a819fd60a..19cc125918bb 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -294,6 +294,45 @@ struct ftrace_likely_data {
/* Are two types/vars the same type (ignoring qualifiers)? */
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
+/**
+ * assert_type - break compile if the first argument's data type and the second
+ * argument's data type are not the same
+ *
+ * @t1: data type or variable
+ * @t2: data type or variable
+ *
+ * The first and second arguments can be data types or variables or mixed (the
+ * first argument is the data type and the second argument is variable or vice
+ * versa). It determines whether the first argument's data type and the second
+ * argument's data type are the same while compiling, and it breaks compile if
+ * the two types are not the same.
+ * See also assert_typable().
+ */
+#define assert_type(t1, t2) _Static_assert(__same_type(t1, t2))
+
+/**
+ * assert_typable - break compile if the first argument's data type and the
+ * second argument's data type are not the same
+ *
+ * @t: data type or variable
+ * @n: data type or variable or constant value
+ *
+ * The first and second arguments can be data types or variables or mixed (the
+ * first argument is the data type and the second argument is variable or vice
+ * versa). Unlike the assert_type() macro, this macro allows a constant value
+ * as the second argument. And if the second argument is a constant value, it
+ * always passes. And it doesn't mean that the types are explicitly the same.
+ * When a constant value is used as the second argument, if you need an
+ * overflow check when assigning a constant value to a variable of the type of
+ * the first argument, you can use the overflows_type() macro. When a constant

I wonder if the overflows_type() check should happen in this test? It
seems weird that assert_typable(u8, 1024) would pass...

Yes, that's right. If a constant is used as an argument here, it seems necessary to check whether an overflow occurs when the constant value is assigned to the target type or target variable.

+ * value is not used as a second argument, it determines whether the first
+ * argument's data type and the second argument's data type are the same while
+ * compiling, and it breaks compile if the two types are not the same.
+ * See also assert_type() and overflows_type().
+ */
+#define assert_typable(t, n) _Static_assert(__builtin_constant_p(n) || \
+ __same_type(t, typeof(n)))

Totally untested -- I'm not sure if this gets the right semantics for
constant expressoins, etc...

static_assert(__builtin_choose_expression(__builtin_constant_p(n), \
overflows_type(n, typeof(t)), \
__same_type(t, typeof(n))))

However, if we change the macro in the form below, the "error: expression in static assertion is not constant" error occurs due to the restriction [1][2] of _Static_assert() as you mentioned.
( overflows_type() internally uses the __builtin_add_overflow() builtin function [3], which returns a bool type.)

#define assert_same_typable(t, n) static_assert( \
__builtin_choose_expr(__builtin_constant_p(n), \
overflows_type(n, typeof(t)) == false, \
__same_type(t, typeof(n))))

Can I have your opinion on the new addition of overflows_type_return_const_expr(), which returns a constant value at compile time to check whether an overflow occurs when assigning a constant value to an argument type?
If it is allowable to add the macro, I would try to use the macro that returns "an integer constant expression" after checking for overflow between the constant value and the argument type at compile time with reference to implemented in the previous version. [4] or [5]

#define assert_same_typable(t, n) static_assert( \
__builtin_choose_expr(__builtin_constant_p(n), \
overflows_type_return_const_expr(n,t) == 0, \
__same_type(t, typeof(n))))

option (1): add is_unsigned_type() and overflows_type_return_const_expr()
#define is_unsigned_type(x) (!is_signed_type(x))
#define overflows_type_return_const_expr(x, T) \
(is_unsigned_type(x) ? \
is_unsigned_type(T) ? \
(sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
: (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) - 1)) ? 1 : 0 \
: is_unsigned_type(T) ? \
((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
: (sizeof(x) > sizeof(T)) ? \
((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
: ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
: 0)

or option (2): modify current __type_half_max(), type_max(), type_min() and add overflows_type_return_const_expr()

#define __type_half_max(x) (((typeof(x))1) << (BITS_PER_TYPE(x) - 1 - is_signed_type(x)))
#define type_max(x) ((typeof(x))((__type_half_max(x) - 1) + __type_half_max(x)))
#define type_min(x) ((typeof(x))((typeof(x))-type_max(x)-(typeof(x))1))
#define overflows_type_return_const_expr(x,T) ( \
is_unsigned_type(x) ? \
x > type_max(T) ? 1 : 0 \
: is_unsigned_type(T) ? \
x < 0 || x > type_max(T) ? 1 : 0 \
: x < type_min(T) || x > type_max(T) ? 1 : 0 )

Also, can you please add KUnit tests for these new helpers into
lib/overflow_kunit.c?

yes the kunit tests for assert_same_typable() and assert_same_type() will be added in the case of normal build in the form below so that the build of other test cases is not interrupted. [6]

And the added overflows_type() and check_assign() and check_assign_user_ptr() macros use the check_add_overflow() macro, and this macro is verified with another test case. Is it necessary to add it?
And if it's okay to add the overflows_type_return_const_expr() macro mentioned above, I'll add the macro in the new version and add a test case.

[1] https://en.cppreference.com/w/c/language/_Static_assert
_Static_assert ( expression , message ) (since C11)

[2] C11 standard (ISO/IEC 9899:2011):
6.7.10 Static assertions

Syntax
1 static_assert-declaration:
_Static_assert ( constant-expression , string-literal ) ;

Constraints
2 The constant expression shall compare unequal to 0.

Semantics
3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message.

[3] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
6.56 Built-in Functions to Perform Arithmetic with Overflow Checking
Built-in Function: bool __builtin_add_overflow (type1 a, type2 b, type3 *res)

[4] https://patchwork.freedesktop.org/patch/494722/?series=104704&rev=6

[5] https://lore.kernel.org/all/52c09fde-f788-4c2b-efdc-d1783dbc0f6c@xxxxxxxxx/

[6]

/* Arg is: type */
#define TEST_ASSERT_SAME_TYPE(t) do { \
typeof(t) __t1 = type_max(t); \
typeof(t) __t2 = type_min(t); \
assert_same_type(t, t); \
assert_same_type(t, __t1); \
assert_same_type(__t1, t); \
assert_same_type(__t1, __t2); \
} while (0)

/* Arg is: type */
#define TEST_ASSERT_SAME_TYPABLE(t) do { \
typeof(t) __t1 = type_max(t); \
typeof(t) __t2 = type_min(t); \
assert_same_typable(t, __t1); \
assert_same_typable(t, type_max(t)); \
assert_same_typable(t, type_min(t)); \
assert_same_typable(__t1, type_max(t)); \
assert_same_typable(__t1, type_min(t)); \
assert_same_typable(__t1, __t2); \
} while (0)

TEST_ASSERT_SAME_TYPE(u8);
TEST_ASSERT_SAME_TYPE(u16);
TEST_ASSERT_SAME_TYPE(u32);
TEST_ASSERT_SAME_TYPE(u64);
TEST_ASSERT_SAME_TYPE(s8);
TEST_ASSERT_SAME_TYPE(s16);
TEST_ASSERT_SAME_TYPE(s32);
TEST_ASSERT_SAME_TYPE(s64);
TEST_ASSERT_SAME_TYPABLE(u8);
TEST_ASSERT_SAME_TYPABLE(u16);
TEST_ASSERT_SAME_TYPABLE(u32);
TEST_ASSERT_SAME_TYPABLE(u64);
TEST_ASSERT_SAME_TYPABLE(s8);
TEST_ASSERT_SAME_TYPABLE(s16);
TEST_ASSERT_SAME_TYPABLE(s32);
TEST_ASSERT_SAME_TYPABLE(s64);

Next message: Vladimir Oltean: "Re: [PATCH net-next 08/13] net: dsa: hellcreek: deny tc-taprio changes to per-tc max SDU"
Previous message: Liang, Kan: "Re: [PATCH] KVM: x86/pmu: Add PEBS support for Intel Sapphire Rapids"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]