[PATCH 0/1] Add missing include in compiler.h

[PATCH 0/1] Add missing include in compiler.h

[Philipp Reisner]

A compilation unit that only includes linux/string.h runs into an undefined
BUILD_BUG_ON_ZERO(). Fixing that.

Philipp Reisner (1):
  compiler.h: Add missing include statement for build_bug.h

 include/linux/compiler.h | 1 +
 1 file changed, 1 insertion(+)

-- 
2.47.0

[PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Philipp Reisner]

compiler.h defines __must_be_array() and __must_be_cstr() and both
expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO().
So far compiler.h lacks to include build_bug.h.

Fix compiler.h by including build_bug.h. With that compiler.h and
build_bug.h depend on each other.

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 include/linux/compiler.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 4d4e23b6e3e7..2d75e4892316 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -3,6 +3,7 @@
 #define __LINUX_COMPILER_H
 
 #include <linux/compiler_types.h>
+#include <linux/build_bug.h>    /* for BUILD_BUG_ON_ZERO() */
 
 #ifndef __ASSEMBLY__
 
-- 
2.47.0

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Dan Carpenter]

On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote:
> compiler.h defines __must_be_array() and __must_be_cstr() and both
> expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO().
> So far compiler.h lacks to include build_bug.h.
> 
> Fix compiler.h by including build_bug.h. With that compiler.h and
> build_bug.h depend on each other.
> 
> Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> ---

Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array")

What actually breaks?  This commit is six years old.  It's weird that we're only
seeing build breakage now.  Or did you just notice this while reviewing the
code?

regards,
dan carpenter

>  include/linux/compiler.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 4d4e23b6e3e7..2d75e4892316 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -3,6 +3,7 @@
>  #define __LINUX_COMPILER_H
>  
>  #include <linux/compiler_types.h>
> +#include <linux/build_bug.h>    /* for BUILD_BUG_ON_ZERO() */
>  
>  #ifndef __ASSEMBLY__
>  
> -- 
> 2.47.0
> 

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Philipp Reisner]

On Thu, Nov 14, 2024 at 11:40 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote:
> > compiler.h defines __must_be_array() and __must_be_cstr() and both
> > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO().
> > So far compiler.h lacks to include build_bug.h.
> >
> > Fix compiler.h by including build_bug.h. With that compiler.h and
> > build_bug.h depend on each other.
> >
> > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> > ---
>
> Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array")
>
> What actually breaks?  This commit is six years old.  It's weird that we're only
> seeing build breakage now.  Or did you just notice this while reviewing the
> code?
>

I am working on a compilation unit that includes linux/string.h.
Compiling it breaks when using strscp(). That is since commit
commit 559048d156ff3391c4b793779a824c9193e20442
Author: Kees Cook <kees@kernel.org>
Date:   Mon Aug 5 14:43:44 2024 -0700

Of course, my trivial workaround is including build_bug.h in my
source; it is just not the proper way to fix this.

best regards,
 Phil

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Dan Carpenter]

On Thu, Nov 14, 2024 at 12:01:42PM +0100, Philipp Reisner wrote:
> On Thu, Nov 14, 2024 at 11:40 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote:
> > > compiler.h defines __must_be_array() and __must_be_cstr() and both
> > > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO().
> > > So far compiler.h lacks to include build_bug.h.
> > >
> > > Fix compiler.h by including build_bug.h. With that compiler.h and
> > > build_bug.h depend on each other.
> > >
> > > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> > > ---
> >
> > Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array")
> >
> > What actually breaks?  This commit is six years old.  It's weird that we're only
> > seeing build breakage now.  Or did you just notice this while reviewing the
> > code?
> >
> 
> I am working on a compilation unit that includes linux/string.h.
> Compiling it breaks when using strscp(). That is since commit
> commit 559048d156ff3391c4b793779a824c9193e20442
> Author: Kees Cook <kees@kernel.org>
> Date:   Mon Aug 5 14:43:44 2024 -0700
> 
> Of course, my trivial workaround is including build_bug.h in my
> source; it is just not the proper way to fix this.
> 

Ah, okay.  Thanks.  I thought it might have broken in tree code.

regards,
dan carpenter

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Linus Torvalds]

On Thu, 14 Nov 2024 at 02:23, Philipp Reisner
<philipp.reisner@linbit.com> wrote:
>
> compiler.h defines __must_be_array() and __must_be_cstr() and both
> expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO().
> So far compiler.h lacks to include build_bug.h.

The bug is real, but..

> Fix compiler.h by including build_bug.h. With that compiler.h and
> build_bug.h depend on each other.

I hate how compiler.h would include build_bug.h, which - on the very
first line - then in turn includes compiler.h.

Does it *work*? Yes. The standard include guards stop the thing from
recursing, and both headers only do create pre-processor defines, so
the dependencies aren't ordered, but it's really really ugly to have
these kinds of circular includes.

I think a better fix would be to not use BUILD_BUG_ON_ZERO() at all,
but just use _Static_assert() directly here, to make
<linux/compiler.h> be more self-sufficient.

The gcc docs say that __builtin_types_compatible_p() and
__builtin_has_attribute() both return an integer constant expression,
so that should be fine (the advantage of BUILD_BUG_ON_ZERO() is that
it works in some contexts that aren't necessarily technically integer
constant expressions - as long as they just evaluate to a constant).

We historically used to avoid _Static_assert(), but that was for
historical reasons (ie it's one of those things that didn't exist back
in the day..)

Hmm?

              Linus

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Linus Torvalds]

On Thu, 14 Nov 2024 at 09:42, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think a better fix would be to not use BUILD_BUG_ON_ZERO() at all,
> but just use _Static_assert() directly here, to make
> <linux/compiler.h> be more self-sufficient.

Damn. We can't do that, because we typically use this in contexts that
require us to return zero (because the assertion part of an
expression), and then that whole expression needs to be a constant
integer expression.

And because _Static_assert() isn't an expression, we'd need to wrap it
with a statement expression or something.

And *hat* we can't do in arbitrary contexts.

Grr. I absolutely detest how bad the standard tools are. It's kind of
sad how we need to build our own hacky BUILD_BUG_ON() to do this.

There's probably some trick I'm missing, but yeah, it looks like we
need our BUILD_BUG_ON_ZERO() thing with that crazy bitfield hack.

                           Linus

Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h

[Linus Torvalds]

On Thu, 14 Nov 2024 at 10:28, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> There's probably some trick I'm missing, but yeah, it looks like we
> need our BUILD_BUG_ON_ZERO() thing with that crazy bitfield hack.

.. and right after sending that, I figured out the trick.

You can use 'static_assert()' inside a type definition.

So the way to turn it into an expression is to just use the same
'sizeof(empty struct)' trick that we use for BUILD_BUG_ON_ZERO:

    #define Static_assert(a,msg) \
        sizeof(struct{_Static_assert(a,msg);})

works as a way to make _Static_assert() be an expression. What a horrid hack.

I don't know if this is worth it, but it does at least have the
advantage of having a message, so that the error case can explain
itself rather than get that odd

 "negative width in bit-field"

error message.

I dunno.

                Linus

[PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()

[Philipp Reisner]

<linux/compiler.h> defines __must_be_array() and __must_be_cstr() and
both expand to BUILD_BUG_ON_ZERO(), but <linux/build_bug.h> defines
BUILD_BUG_ON_ZERO(). Including <linux/build_bug.h> in
<linux/compiler.h> would create a cyclic dependency as
<linux/build_bug.h> already includes <linux/compiler.h>.

Fix that by defining __BUILD_BUG_ON_ZERO_MSG() in <linux/compiler.h>
and using that for __must_be_array() and __must_be_cstr().

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 include/linux/compiler.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 4d4e23b6e3e7..469a64dd6495 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -239,11 +239,18 @@ static inline void *offset_to_ptr(const int *off)
 
 #endif /* __ASSEMBLY__ */
 
+#ifdef __CHECKER__
+#define __BUILD_BUG_ON_ZERO_MSG(e, msg) (0)
+#else /* __CHECKER__ */
+#define __BUILD_BUG_ON_ZERO_MSG(e, msg) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
+#endif /* __CHECKER__ */
+
 /* &a[0] degrades to a pointer: a different type from an array */
-#define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(__same_type((a), &(a)[0]), "must be array")
 
 /* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
-#define __must_be_cstr(p)	BUILD_BUG_ON_ZERO(__annotated(p, nonstring))
+#define __must_be_cstr(p) \
+	__BUILD_BUG_ON_ZERO_MSG(__annotated(p, nonstring), "must be cstr (NUL-terminated)")
 
 /*
  * This returns a constant expression while determining if an argument is
-- 
2.47.0

Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()

[Kees Cook]

On Fri, Nov 15, 2024 at 09:46:02PM +0100, Philipp Reisner wrote:
> <linux/compiler.h> defines __must_be_array() and __must_be_cstr() and
> both expand to BUILD_BUG_ON_ZERO(), but <linux/build_bug.h> defines
> BUILD_BUG_ON_ZERO(). Including <linux/build_bug.h> in
> <linux/compiler.h> would create a cyclic dependency as
> <linux/build_bug.h> already includes <linux/compiler.h>.
> 
> Fix that by defining __BUILD_BUG_ON_ZERO_MSG() in <linux/compiler.h>
> and using that for __must_be_array() and __must_be_cstr().
> 
> Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>

Thanks for finding a simple way to make this work sanely. :)

Acked-by: Kees Cook <kees@kernel.org>

Linus, do you want a PR for this, or will you apply it directly?

Thanks!

-Kees

> ---
>  include/linux/compiler.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 4d4e23b6e3e7..469a64dd6495 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -239,11 +239,18 @@ static inline void *offset_to_ptr(const int *off)
>  
>  #endif /* __ASSEMBLY__ */
>  
> +#ifdef __CHECKER__
> +#define __BUILD_BUG_ON_ZERO_MSG(e, msg) (0)
> +#else /* __CHECKER__ */
> +#define __BUILD_BUG_ON_ZERO_MSG(e, msg) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
> +#endif /* __CHECKER__ */
> +
>  /* &a[0] degrades to a pointer: a different type from an array */
> -#define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> +#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(__same_type((a), &(a)[0]), "must be array")
>  
>  /* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
> -#define __must_be_cstr(p)	BUILD_BUG_ON_ZERO(__annotated(p, nonstring))
> +#define __must_be_cstr(p) \
> +	__BUILD_BUG_ON_ZERO_MSG(__annotated(p, nonstring), "must be cstr (NUL-terminated)")
>  
>  /*
>   * This returns a constant expression while determining if an argument is
> -- 
> 2.47.0
> 

-- 
Kees Cook

Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()

[Linus Torvalds]

On Fri, 15 Nov 2024 at 12:48, Kees Cook <kees@kernel.org> wrote:
>
> Linus, do you want a PR for this, or will you apply it directly?

Well, I'm certainly not applying it this late for 6.12 - who knows
what compiler issues it can trigger - and for the merge window I will
have forgotten it.

So put it in your tree and have it go through linux-next to see that
it's ok. It *looks* fine to me, but...

            Linus

Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()

[Linus Torvalds]

On Fri, 15 Nov 2024 at 12:46, Philipp Reisner
<philipp.reisner@linbit.com> wrote:
>
> Fix that by defining __BUILD_BUG_ON_ZERO_MSG() in <linux/compiler.h>
> and using that for __must_be_array() and __must_be_cstr().

Ack, that cast to 'int' seems good too, to make sure the
__BUILD_BUG_ON_ZERO_MSG() test doesn't unintentionally change the type
of the expression it is in.

I do wonder if we actually need that "#ifdef __CHECKER__"? I think
sparse is perfectly fine with a _Static_assert(). Or does the checking
cause some other issues?

            Linus

Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()

[Kees Cook]

On Fri, 15 Nov 2024 21:46:02 +0100, Philipp Reisner wrote:
> <linux/compiler.h> defines __must_be_array() and __must_be_cstr() and
> both expand to BUILD_BUG_ON_ZERO(), but <linux/build_bug.h> defines
> BUILD_BUG_ON_ZERO(). Including <linux/build_bug.h> in
> <linux/compiler.h> would create a cyclic dependency as
> <linux/build_bug.h> already includes <linux/compiler.h>.
> 
> Fix that by defining __BUILD_BUG_ON_ZERO_MSG() in <linux/compiler.h>
> and using that for __must_be_array() and __must_be_cstr().
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
      https://git.kernel.org/kees/c/d7a516c6eeae

Take care,

-- 
Kees Cook