| Message ID | 20250201200503.2532357-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] alloc_tag: work around clang-14 issue with __builtin_object_size() | expand |
On Sat, Feb 01, 2025 at 12:05:03PM -0800, Suren Baghdasaryan wrote: > Additional condition in the allocation hooks causes Clang version 14 > (tested on 14.0.6) to treat the allocated object size as unknown at > compile-time (__builtin_object_size(obj, 1) returns -1) even though > both branches of that condition yield the same result. Other versions > of Clang (tested with 13.0.1, 15.0.7, 16.0.6 and 17.0.6) compile the > same code without issues. Add build-time Clang version check which > removes this condition and effectively restores the unconditional tag > store/restore flow when compiled with clang-14. > > Fixes: 07438779313c ("alloc_tag: avoid current->alloc_tag manipulations when profiling is disabled") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/ > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/alloc_tag.h | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h > index a946e0203e6d..df432c2c3483 100644 > --- a/include/linux/alloc_tag.h > +++ b/include/linux/alloc_tag.h > @@ -222,10 +222,23 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {} > > #endif /* CONFIG_MEM_ALLOC_PROFILING */ > > +/* See https://lore.kernel.org/all/202501310832.kiAeOt2z-lkp@intel.com/ */ > +#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION >= 140000 && CONFIG_CLANG_VERSION < 150000 FWIW, this could just be "< 150000" -- < 14 doesn't warn because (as Nathan mentioned to me today) it didn't support the build-time error attribute, so it wouldn't have warned even if it did trip over it. > +static inline bool store_current_tag(void) > +{ > + return true; > +} > +#else > +static inline bool store_current_tag(void) > +{ > + return mem_alloc_profiling_enabled(); > +} > +#endif > + > #define alloc_hooks_tag(_tag, _do_alloc) \ > ({ \ > typeof(_do_alloc) _res; \ > - if (mem_alloc_profiling_enabled()) { \ > + if (store_current_tag()) { \ > struct alloc_tag * __maybe_unused _old; \ > _old = alloc_tag_save(_tag); \ > _res = _do_alloc; \ I think the work-around is fine, but I'm trying to dig into the root cause here. As you found, it fails on the final strtomem_pad: strtomem_pad(key->u.kbd.press_str, press, '\0'); strtomem_pad(key->u.kbd.repeat_str, repeat, '\0'); strtomem_pad(key->u.kbd.release_str, release, '\0'); (but not the earlier calls??) The destinations are: char press_str[sizeof(void *) + sizeof(int)] __nonstring; char repeat_str[sizeof(void *) + sizeof(int)] __nonstring; char release_str[sizeof(void *) + sizeof(int)] __nonstring; Random thoughts include "this is the last array in the struct" which might imply bad compiler behavior about its sizing via __builtin_object_size() (i.e. trailing array must always be unknown size to deal with fake flex arrays), but that wasn't fixed until Clang 16 (with -fstrict-flex-arrays=3), so that it doesn't trip in Clang 15 is odd. To Kent's comment[1], I believe I was using __builtin_object_size() here because I have a knee-jerk aversion to sizeof() due to it blowing up on flexible arrays, but that's not relevant here. ARRAY_SIZE() would work, but only if type checking to "char *" succeeds, as Kent suggests. Let me see if making those changes survives testing... -Kees [1] https://lore.kernel.org/all/qbmazoiryyqygjk2x6bc7puqvmik7gyitzo3xnryzsodnrrjek@tahia33lvpli/ > > base-commit: 60c828cf80c07394762a1edfaff63bea55cc8e45 > -- > 2.48.1.362.g079036d154-goog >
On Wed, Feb 05, 2025 at 11:18:35AM -0800, Kees Cook wrote: > On Sat, Feb 01, 2025 at 12:05:03PM -0800, Suren Baghdasaryan wrote: > > Additional condition in the allocation hooks causes Clang version 14 > > (tested on 14.0.6) to treat the allocated object size as unknown at > > compile-time (__builtin_object_size(obj, 1) returns -1) even though > > both branches of that condition yield the same result. Other versions > > of Clang (tested with 13.0.1, 15.0.7, 16.0.6 and 17.0.6) compile the > > same code without issues. Add build-time Clang version check which > > removes this condition and effectively restores the unconditional tag > > store/restore flow when compiled with clang-14. > > > > Fixes: 07438779313c ("alloc_tag: avoid current->alloc_tag manipulations when profiling is disabled") > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/ > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/alloc_tag.h | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h > > index a946e0203e6d..df432c2c3483 100644 > > --- a/include/linux/alloc_tag.h > > +++ b/include/linux/alloc_tag.h > > @@ -222,10 +222,23 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {} > > > > #endif /* CONFIG_MEM_ALLOC_PROFILING */ > > > > +/* See https://lore.kernel.org/all/202501310832.kiAeOt2z-lkp@intel.com/ */ > > +#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION >= 140000 && CONFIG_CLANG_VERSION < 150000 > > FWIW, this could just be "< 150000" -- < 14 doesn't warn because (as > Nathan mentioned to me today) it didn't support the build-time error > attribute, so it wouldn't have warned even if it did trip over it. > > > +static inline bool store_current_tag(void) > > +{ > > + return true; > > +} > > +#else > > +static inline bool store_current_tag(void) > > +{ > > + return mem_alloc_profiling_enabled(); > > +} > > +#endif > > + > > #define alloc_hooks_tag(_tag, _do_alloc) \ > > ({ \ > > typeof(_do_alloc) _res; \ > > - if (mem_alloc_profiling_enabled()) { \ > > + if (store_current_tag()) { \ > > struct alloc_tag * __maybe_unused _old; \ > > _old = alloc_tag_save(_tag); \ > > _res = _do_alloc; \ > > I think the work-around is fine, but I'm trying to dig into the root > cause here. > > As you found, it fails on the final strtomem_pad: > > strtomem_pad(key->u.kbd.press_str, press, '\0'); > strtomem_pad(key->u.kbd.repeat_str, repeat, '\0'); > strtomem_pad(key->u.kbd.release_str, release, '\0'); > > (but not the earlier calls??) The destinations are: > > char press_str[sizeof(void *) + sizeof(int)] __nonstring; > char repeat_str[sizeof(void *) + sizeof(int)] __nonstring; > char release_str[sizeof(void *) + sizeof(int)] __nonstring; > > Random thoughts include "this is the last array in the struct" which might > imply bad compiler behavior about its sizing via __builtin_object_size() > (i.e. trailing array must always be unknown size to deal with > fake flex arrays), but that wasn't fixed until Clang 16 (with > -fstrict-flex-arrays=3), so that it doesn't trip in Clang 15 is odd. I bisected the fix in LLVM 15 to https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf90829951ddc, which certainly makes sense. Given the commit mentions phi nodes and folding means that maybe there is a branch that was not getting eliminated before this change? I have not really looked into the call chain here. > To Kent's comment[1], I believe I was using __builtin_object_size() here > because I have a knee-jerk aversion to sizeof() due to it blowing up on > flexible arrays, but that's not relevant here. ARRAY_SIZE() would work, > but only if type checking to "char *" succeeds, as Kent suggests. > > Let me see if making those changes survives testing... If that suggestion works, I would certainly prefer that to a compiler version workaround. Worst case, we could bump the minimum supported LLVM version over this but it does not seem serious enough to do so at the moment. Cheers, Nathan
On Wed, Feb 05, 2025 at 11:18:35AM -0800, Kees Cook wrote: > On Sat, Feb 01, 2025 at 12:05:03PM -0800, Suren Baghdasaryan wrote: > > Additional condition in the allocation hooks causes Clang version 14 > > (tested on 14.0.6) to treat the allocated object size as unknown at > > compile-time (__builtin_object_size(obj, 1) returns -1) even though > > both branches of that condition yield the same result. Other versions > > of Clang (tested with 13.0.1, 15.0.7, 16.0.6 and 17.0.6) compile the > > same code without issues. Add build-time Clang version check which > > removes this condition and effectively restores the unconditional tag > > store/restore flow when compiled with clang-14. > > > > Fixes: 07438779313c ("alloc_tag: avoid current->alloc_tag manipulations when profiling is disabled") > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/ > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/alloc_tag.h | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h > > index a946e0203e6d..df432c2c3483 100644 > > --- a/include/linux/alloc_tag.h > > +++ b/include/linux/alloc_tag.h > > @@ -222,10 +222,23 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {} > > > > #endif /* CONFIG_MEM_ALLOC_PROFILING */ > > > > +/* See https://lore.kernel.org/all/202501310832.kiAeOt2z-lkp@intel.com/ */ > > +#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION >= 140000 && CONFIG_CLANG_VERSION < 150000 > > FWIW, this could just be "< 150000" -- < 14 doesn't warn because (as > Nathan mentioned to me today) it didn't support the build-time error > attribute, so it wouldn't have warned even if it did trip over it. > > > +static inline bool store_current_tag(void) > > +{ > > + return true; > > +} > > +#else > > +static inline bool store_current_tag(void) > > +{ > > + return mem_alloc_profiling_enabled(); > > +} > > +#endif > > + > > #define alloc_hooks_tag(_tag, _do_alloc) \ > > ({ \ > > typeof(_do_alloc) _res; \ > > - if (mem_alloc_profiling_enabled()) { \ > > + if (store_current_tag()) { \ > > struct alloc_tag * __maybe_unused _old; \ > > _old = alloc_tag_save(_tag); \ > > _res = _do_alloc; \ > > I think the work-around is fine, but I'm trying to dig into the root > cause here. > > As you found, it fails on the final strtomem_pad: > > strtomem_pad(key->u.kbd.press_str, press, '\0'); > strtomem_pad(key->u.kbd.repeat_str, repeat, '\0'); > strtomem_pad(key->u.kbd.release_str, release, '\0'); > > (but not the earlier calls??) The destinations are: > > char press_str[sizeof(void *) + sizeof(int)] __nonstring; > char repeat_str[sizeof(void *) + sizeof(int)] __nonstring; > char release_str[sizeof(void *) + sizeof(int)] __nonstring; > > Random thoughts include "this is the last array in the struct" which might > imply bad compiler behavior about its sizing via __builtin_object_size() > (i.e. trailing array must always be unknown size to deal with > fake flex arrays), but that wasn't fixed until Clang 16 (with > -fstrict-flex-arrays=3), so that it doesn't trip in Clang 15 is odd. > > To Kent's comment[1], I believe I was using __builtin_object_size() here > because I have a knee-jerk aversion to sizeof() due to it blowing up on > flexible arrays, but that's not relevant here. ARRAY_SIZE() would work, > but only if type checking to "char *" succeeds, as Kent suggests. Yeah, that rational for __builtin_object_size() makes sense - although it's not what the gcc docs say, those talk about getting the size from an attribute on the allocation function (!). ARRAY_SIZE() is sizeof() underneath, just used creatively to guarantee that the input is an array - although that property is probably what we want here, since strtomem_pad() really only makes sense on static or flex-arrays, no?
On Wed, Feb 05, 2025 at 03:16:12PM -0500, Kent Overstreet wrote: > ARRAY_SIZE() is sizeof() underneath, just used creatively to guarantee > that the input is an array - although that property is probably what we > want here, since strtomem_pad() really only makes sense on static or > flex-arrays, no? Exactly. strtomem*/memtostr* are very picky about the destination being compile-time sized, so sizeof() under the hood seems correct. I'm making my way through testing a patch now...
On Wed, Feb 05, 2025 at 03:16:12PM -0500, Kent Overstreet wrote: > On Wed, Feb 05, 2025 at 11:18:35AM -0800, Kees Cook wrote: > > On Sat, Feb 01, 2025 at 12:05:03PM -0800, Suren Baghdasaryan wrote: > > To Kent's comment[1], I believe I was using __builtin_object_size() here > > because I have a knee-jerk aversion to sizeof() due to it blowing up on > > flexible arrays, but that's not relevant here. ARRAY_SIZE() would work, > > but only if type checking to "char *" succeeds, as Kent suggests. > > Yeah, that rational for __builtin_object_size() makes sense - although > it's not what the gcc docs say, those talk about getting the size from > an attribute on the allocation function (!). > > ARRAY_SIZE() is sizeof() underneath, just used creatively to guarantee > that the input is an array - although that property is probably what we > want here, since strtomem_pad() really only makes sense on static or > flex-arrays, no? Okay, here's my proposed fix, and confirmed that it solves the problem: https://lore.kernel.org/lkml/20250206175216.work.225-kees@kernel.org -Kees
diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h index a946e0203e6d..df432c2c3483 100644 --- a/include/linux/alloc_tag.h +++ b/include/linux/alloc_tag.h @@ -222,10 +222,23 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {} #endif /* CONFIG_MEM_ALLOC_PROFILING */ +/* See https://lore.kernel.org/all/202501310832.kiAeOt2z-lkp@intel.com/ */ +#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION >= 140000 && CONFIG_CLANG_VERSION < 150000 +static inline bool store_current_tag(void) +{ + return true; +} +#else +static inline bool store_current_tag(void) +{ + return mem_alloc_profiling_enabled(); +} +#endif + #define alloc_hooks_tag(_tag, _do_alloc) \ ({ \ typeof(_do_alloc) _res; \ - if (mem_alloc_profiling_enabled()) { \ + if (store_current_tag()) { \ struct alloc_tag * __maybe_unused _old; \ _old = alloc_tag_save(_tag); \ _res = _do_alloc; \
Additional condition in the allocation hooks causes Clang version 14 (tested on 14.0.6) to treat the allocated object size as unknown at compile-time (__builtin_object_size(obj, 1) returns -1) even though both branches of that condition yield the same result. Other versions of Clang (tested with 13.0.1, 15.0.7, 16.0.6 and 17.0.6) compile the same code without issues. Add build-time Clang version check which removes this condition and effectively restores the unconditional tag store/restore flow when compiled with clang-14. Fixes: 07438779313c ("alloc_tag: avoid current->alloc_tag manipulations when profiling is disabled") Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/ Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/alloc_tag.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) base-commit: 60c828cf80c07394762a1edfaff63bea55cc8e45