[PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Suren Baghdasaryan]

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
+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;					\

base-commit: 60c828cf80c07394762a1edfaff63bea55cc8e45
-- 
2.48.1.362.g079036d154-goog

Re: [PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Kees Cook]

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
> 

-- 
Kees Cook

Re: [PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Nathan Chancellor]

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

Re: [PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Kent Overstreet]

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?

Re: [PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Kees Cook]

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...

-- 
Kees Cook

Re: [PATCH 1/1] alloc_tag: work around clang-14 issue with __builtin_object_size()

[Kees Cook]

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

-- 
Kees Cook