| Message ID | 20220824084514.2261614-3-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation | expand |
Hi Bartosz Golaszewski, would you mind taking a look at this patch? Thanks, G.G. On 8/24/22 5:45 PM, 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))) > + > +/** > + * 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) > + > #endif
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? 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)) > + > +/** > + * 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. :)
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. :) >
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))) + +/** + * 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) + #endif
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(+)