Re: [PATCH v9 2/8] util_macros: Add exact_type macro to catch type mis-match while compiling

[Gwan-gyeong Mun]

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@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>
> > ---
> >   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. :)