Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5814cf7f-00ec-06d4-089a-41b173eed8b7@intel.com>
Date:   Fri, 9 Sep 2022 19:58:45 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v9 2/8] util_macros: Add exact_type macro to catch type
 mis-match while compiling
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>
CC:     <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <mchehab@kernel.org>,
        <chris@chris-wilson.co.uk>, <matthew.auld@intel.com>,
        <thomas.hellstrom@linux.intel.com>, <jani.nikula@intel.com>,
        <nirmoy.das@intel.com>, <airlied@linux.ie>, <daniel@ffwll.ch>,
        <andi.shyti@linux.intel.com>, <andrzej.hajda@intel.com>,
        <mauro.chehab@linux.intel.com>,
        <intel-gfx-trybot@lists.freedesktop.org>
References: <20220824084514.2261614-1-gwan-gyeong.mun@intel.com>
 <20220824084514.2261614-3-gwan-gyeong.mun@intel.com>
 <202208250953.A71FEE45E@keescook>
From:   Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
In-Reply-To: <202208250953.A71FEE45E@keescook>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YzZOVVNCMEp3SEtsUHFBRFpGOEE2OUdaTXJsdXRLSEROb1E0WldQRXM5V0VH?=
 =?utf-8?B?OTRzZU5Nc0JMM2ZLWlBDbGdEZXRiNTRRTjhkM3F3WXJqYXJtRnVYV0hxZU5X?=
 =?utf-8?B?TlJJZnNKRy9oTUx6N2xpRUk5Y3JMc0IxS3pFMVNVbS8ybmhYNUFuL1pqY0NJ?=
 =?utf-8?B?UkRBNDNtNEdMYjV4RHErR1pIN29lOGg4cW5Jb29QM3VCaVNrejFteDJPaDR0?=
 =?utf-8?B?bmlQeG9pd1c0V3NWQzRGUFVJUmdCZURaVHY5bHdnNytvMm1IT0tuN3lJd0Ey?=
 =?utf-8?B?N094d2Jia1JSeGFZRTQvV1NRM1o5Q1RTRklvMHI0RkEyZ2t3QVg5aEJkcHV4?=
 =?utf-8?B?NW9JRWxJaVN1TG96TVIyYjlUQlowbmRGTDdReHRsQUhqbThqSEFuWTF3UnJx?=
 =?utf-8?B?QUozTXRqeGVQNGdzMlQ3TGJubGhVS1VKTHZlUE1JSEp2RnVSWCtYVUZrc2c2?=
 =?utf-8?B?K28xNCtBcnFWR2RFNmg2clNOUWdFNVRMTURZb3EyWDE0anl1Y3ZYTkd4Qk9j?=
 =?utf-8?B?OEtUT0d1Y0lvdEgrZ3VLaitnUzdKL05rSnFTY1RPUkRSR0lScis1ZUpYV1hD?=
 =?utf-8?B?M2wvOVAvOGFQZVZ4NjFYRkhMTXRTZi92bGpRek5vNGdZTVlwSVFyZnhuNTNL?=
 =?utf-8?B?dEZxNzFqeGlZQ0U4RHhqUW1UKy9ML21DVmhMbjQ1TlVwaEhQRGJaSWM3TjY3?=
 =?utf-8?B?VTQ1eC9PNEUyRkpDTFZ1R21ZYWFtM1dQYkFYbUx6TXFnUjVVTjJVRThFYTVL?=
 =?utf-8?B?eDFPVnhqMDlIRE03OFNiWmkxUm8yeFU0VEdzUmZ1WktEc1RNbGhuVDdnZHpO?=
 =?utf-8?B?ekhLN2krbVlWQmNFVXFjZ04vNTZMK1hnaGJhOXduWEpPbUx5d2pyT3h5WStV?=
 =?utf-8?B?Z1hzYWlkNTNJUVNKMU9xTDRCcjRycEg5aHFhdlFmblJ4cEMraTRQQUpLY21l?=
 =?utf-8?B?S29yOVp5WDcxVlh2WVU3WVQ4SVhhQk14eUt2NHcvVnMrWDhZYXBISWZCYXcr?=
 =?utf-8?B?eTBQZm03elZ0cWhLcXBKWGhYMzFRWnBCV2pQaUlWVlcybStzV3djREIzb0pi?=
 =?utf-8?B?ellnTDVXWVhaRGJST0xzSHZPd2xaVFpjcWFjUElkaHRMeTBYd1lnY3B0K1Uy?=
 =?utf-8?B?cHFvUXhab016bFk5MkhsUE01TVl2TE4xa29tQ3pWb0dRRlFsdFdZYjVYOEY0?=
 =?utf-8?B?TW03cE1YeEJGSUFoLzVyZGc5VUpsckNmRWc4L3pVSkIwamlObUp5WXhoM092?=
 =?utf-8?B?UjJ5V0lFUkxucnFVdldEQmcrYTlCdG1MSVNTNkw5a09hbDg3UG9vTGhqNkQ0?=
 =?utf-8?B?OFdDczdUeGFqbDUwSG5OZHNsUkhhOEFyWFdsUlFpQjljYlk1VFcvT1FnUE9Q?=
 =?utf-8?B?eDFXVEV1OVBCSzEwNERTQ05EbnZmZEZOa1o0OTFuM2t5MzlydEVpeWtUUm1t?=
 =?utf-8?B?RjNTNnlXUWUwQjZPYlZGdCt3T3BzdElnTkIyUTc4RktWZ3pyVGN3dWJBOFVr?=
 =?utf-8?B?bUFDcHl2K3A3b1hGdnNDNXRYL0g2emhtbWVTdG5RSGova1pKenJtOERkdG01?=
 =?utf-8?B?YnNMSXNXdjZBUjd4WVlqS1ZLcXFKaEpyYmpvcGNLWDIyM29sV2N4bTJUTTBT?=
 =?utf-8?B?WHpoODY5ak5WTEh5TlNRbk1hb1dEU2JWcjJ5a0xCczNodng1QmJkK3MyaEJh?=
 =?utf-8?B?OGpuQlVTVTI1MG5vNnRvcjdNTDQwVzA0MHplODFEU3p0SmJIemxrVUZwSlg5?=
 =?utf-8?B?WnIzZHV2aGxmeTZydGtUYzRhcWhNeU5VOXpSL2tSenE2WXBiSGVFRkduODNa?=
 =?utf-8?B?b3cwZ2Z5QkUzZXhVRWQyeE83RDJTS1FzQ3Q1aEpnUWFxM2tjUHh0enhwTnFR?=
 =?utf-8?B?cjRmekR6OElIWmZpRmVqMkY1L0x1OFlmaHpkNTk2STNjM1NkdndHaXZoRSsx?=
 =?utf-8?B?eUJETkVIcVlWNTlnUHdtTmR3bDU2S3JOeGdIS0hTL0VvSDhrN1QvUE5MYWlU?=
 =?utf-8?B?ZkxBQk8zOEZUclNZeThPb3lpT2xKdElYQS9NNUJBNnhJM0RMZTd4UVh4K3BF?=
 =?utf-8?B?NEdxajZBUlpibDhsQ29xZUhPK1VUa0tFN2EvYzQzOG1QQ3N3V0dXWS9VRzFS?=
 =?utf-8?B?OWpua2hkckdQV1JFYm8rVEIxanQwSmNyR3ZaTCszUjJIbUZJQkFPa044SzRn?=
 =?utf-8?B?dFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ae66d155-38b6-4ef0-183a-08da92525203
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB5409.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2022 10:59:08.9753
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: s9U/uwCWcDrqYAC9mJTj0VNgNm2aG7i0rZed0eMjbZD+yAkD46FeB3U8Qrp2YViYUPAq1cLfHhjG4wzIG4e0e3lvgvLbOdYKD+t8SLlACeM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6720
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 8/26/22 2:19 AM, Kees Cook wrote:
> On Wed, Aug 24, 2022 at 05:45:08PM +0900, Gwan-gyeong Mun wrote:
>> It adds exact_type and exactly_pgoff_t macro to catch type mis-match while
>> compiling. The existing typecheck() macro outputs build warnings, but the
>> newly added exact_type() macro uses the BUILD_BUG_ON() macro to generate
>> a build break when the types are different and can be used to detect
>> explicit build errors.
>>
>> v6: Move macro addition location so that it can be used by other than drm
>>      subsystem (Jani, Mauro, Andi)
>>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> ---
>>   include/linux/util_macros.h | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/include/linux/util_macros.h b/include/linux/util_macros.h
>> index 72299f261b25..b6624b275257 100644
>> --- a/include/linux/util_macros.h
>> +++ b/include/linux/util_macros.h
>> @@ -2,6 +2,9 @@
>>   #ifndef _LINUX_HELPER_MACROS_H_
>>   #define _LINUX_HELPER_MACROS_H_
>>   
>> +#include <linux/types.h>
>> +#include <linux/bug.h>
>> +
>>   #define __find_closest(x, a, as, op)					\
>>   ({									\
>>   	typeof(as) __fc_i, __fc_as = (as) - 1;				\
>> @@ -38,4 +41,26 @@
>>    */
>>   #define find_closest_descending(x, a, as) __find_closest(x, a, as, >=)
>>   
>> +/**
>> + * exact_type - break compile if source type and destination value's type are
>> + * not the same
>> + * @T: Source type
>> + * @n: Destination value
>> + *
>> + * It is a helper macro for a poor man's -Wconversion: only allow variables of
>> + * an exact type. It determines whether the source type and destination value's
>> + * type are the same while compiling, and it breaks compile if two types are
>> + * not the same
>> + */
>> +#define exact_type(T, n) \
>> +	BUILD_BUG_ON(!__builtin_constant_p(n) && !__builtin_types_compatible_p(T, typeof(n)))
> 
> Maybe use __same_type() here instead of open-coded
> __builtin_types_compatible_p()? Also, IIUC, currently coding style
> advise is to use _Static_assert when possible over BUILD_BUG_ON for
> error message readability.
> 
> This macro has a trap-door for literals, yes?
> i.e.  exact_type(pgoff_t, 5) will pass?
> 
yes, I will update in detail comments about trap-door that may occur 
when using constant value.

> I also note that this is very close to the really common (and open-coded)
> test scattered around the kernel already (BUILD_BUG_ON(__same_type(a,
> b))), so I think it's good to get a macro defined for it, though I'm not
> sure about the trap door test. Regardless, I'd like to bikeshed the name
> a bit; I think this should be named something a bit more clear about
> what happens on failure. Perhaps: assert_type()? Or to capture the
> trapdoor idea, assert_typable()?
> 
> #define assert_type(t1, t2)	_Static_assert(__same_type(t1, t2))
> #define assert_typable(t, n)	_Static_assert(__builtin_constant_p(n) ||
> 					       __same_type(t, typeof(n))

The form of the assert_type() / assert_typable() macros you suggested 
looks better to me, so I will add these macros to the header where 
__same_type() is defined and will send a new version of the patch.

many thanks
> 
>> +
>> +/**
>> + * exactly_pgoff_t - helper to check if the type of a value is pgoff_t
>> + * @n: value to compare pgoff_t type
>> + *
>> + * It breaks compile if the argument value's type is not pgoff_t type.
>> + */
>> +#define exactly_pgoff_t(n) exact_type(pgoff_t, n)
> 
> Why specialize this? Just use assert_typable(pgoff_t, n) in the other
> patches? It's almost the same amount to write. :)
>