* [PATCH 0/1] Add missing include in compiler.h
@ 2024-11-14 10:14 Philipp Reisner
2024-11-14 10:14 ` [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h Philipp Reisner
0 siblings, 1 reply; 13+ messages in thread
From: Philipp Reisner @ 2024-11-14 10:14 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 10:14 [PATCH 0/1] Add missing include in compiler.h Philipp Reisner
@ 2024-11-14 10:14 ` Philipp Reisner
2024-11-14 10:40 ` Dan Carpenter
2024-11-14 17:42 ` Linus Torvalds
0 siblings, 2 replies; 13+ messages in thread
From: Philipp Reisner @ 2024-11-14 10:14 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: linux-sparse, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 10:14 ` [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h Philipp Reisner
@ 2024-11-14 10:40 ` Dan Carpenter
2024-11-14 11:01 ` Philipp Reisner
2024-11-14 17:42 ` Linus Torvalds
1 sibling, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2024-11-14 10:40 UTC (permalink / raw)
To: Philipp Reisner, Miguel Ojeda
Cc: Luc Van Oostenryck, linux-sparse, linux-kernel
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 10:40 ` Dan Carpenter
@ 2024-11-14 11:01 ` Philipp Reisner
2024-11-14 11:05 ` Dan Carpenter
0 siblings, 1 reply; 13+ messages in thread
From: Philipp Reisner @ 2024-11-14 11:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: Miguel Ojeda, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 11:01 ` Philipp Reisner
@ 2024-11-14 11:05 ` Dan Carpenter
0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2024-11-14 11:05 UTC (permalink / raw)
To: Philipp Reisner
Cc: Miguel Ojeda, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 10:14 ` [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h Philipp Reisner
2024-11-14 10:40 ` Dan Carpenter
@ 2024-11-14 17:42 ` Linus Torvalds
2024-11-14 18:28 ` Linus Torvalds
1 sibling, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2024-11-14 17:42 UTC (permalink / raw)
To: Philipp Reisner, Kees Cook; +Cc: Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 17:42 ` Linus Torvalds
@ 2024-11-14 18:28 ` Linus Torvalds
2024-11-14 18:40 ` Linus Torvalds
0 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2024-11-14 18:28 UTC (permalink / raw)
To: Philipp Reisner, Kees Cook; +Cc: Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h
2024-11-14 18:28 ` Linus Torvalds
@ 2024-11-14 18:40 ` Linus Torvalds
2024-11-15 20:46 ` [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO() Philipp Reisner
0 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2024-11-14 18:40 UTC (permalink / raw)
To: Philipp Reisner, Kees Cook; +Cc: Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
2024-11-14 18:40 ` Linus Torvalds
@ 2024-11-15 20:46 ` Philipp Reisner
2024-11-15 20:48 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Philipp Reisner @ 2024-11-15 20:46 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Kees Cook, Luc Van Oostenryck, linux-sparse, linux-kernel
<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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
2024-11-15 20:46 ` [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO() Philipp Reisner
@ 2024-11-15 20:48 ` Kees Cook
2024-11-15 21:58 ` Linus Torvalds
2024-11-15 21:57 ` Linus Torvalds
2024-11-17 5:26 ` Kees Cook
2 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2024-11-15 20:48 UTC (permalink / raw)
To: Philipp Reisner
Cc: Linus Torvalds, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
2024-11-15 20:46 ` [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO() Philipp Reisner
2024-11-15 20:48 ` Kees Cook
@ 2024-11-15 21:57 ` Linus Torvalds
2024-11-17 5:26 ` Kees Cook
2 siblings, 0 replies; 13+ messages in thread
From: Linus Torvalds @ 2024-11-15 21:57 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Kees Cook, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
2024-11-15 20:48 ` Kees Cook
@ 2024-11-15 21:58 ` Linus Torvalds
0 siblings, 0 replies; 13+ messages in thread
From: Linus Torvalds @ 2024-11-15 21:58 UTC (permalink / raw)
To: Kees Cook; +Cc: Philipp Reisner, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO()
2024-11-15 20:46 ` [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO() Philipp Reisner
2024-11-15 20:48 ` Kees Cook
2024-11-15 21:57 ` Linus Torvalds
@ 2024-11-17 5:26 ` Kees Cook
2 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2024-11-17 5:26 UTC (permalink / raw)
To: Linus Torvalds, Philipp Reisner
Cc: Kees Cook, Luc Van Oostenryck, linux-sparse, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-11-17 5:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-14 10:14 [PATCH 0/1] Add missing include in compiler.h Philipp Reisner
2024-11-14 10:14 ` [PATCH 1/1] compiler.h: Add missing include statement for build_bug.h Philipp Reisner
2024-11-14 10:40 ` Dan Carpenter
2024-11-14 11:01 ` Philipp Reisner
2024-11-14 11:05 ` Dan Carpenter
2024-11-14 17:42 ` Linus Torvalds
2024-11-14 18:28 ` Linus Torvalds
2024-11-14 18:40 ` Linus Torvalds
2024-11-15 20:46 ` [PATCH] compiler.h: Fix undefined BUILD_BUG_ON_ZERO() Philipp Reisner
2024-11-15 20:48 ` Kees Cook
2024-11-15 21:58 ` Linus Torvalds
2024-11-15 21:57 ` Linus Torvalds
2024-11-17 5:26 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).