linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse
@ 2018-11-19 10:31 Masahiro Yamada
  2018-11-19 10:31 ` [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON() Masahiro Yamada
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Masahiro Yamada @ 2018-11-19 10:31 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Luc Van Oostenryck, Nick Desaulniers, Kees Cook, Josh Triplett,
	Masahiro Yamada, Miguel Ojeda, Arnd Bergmann, linux-kernel,
	Paul Burton

When I tried to delete BUILD_BUG_ON stubs for sparse, the kbuild test
robot reported lots of Sparse warnings from container_of(), which
seem false positive.

The following checker in container_of() seems to be causing something
strange for Sparse.

  BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
                   !__same_type(*(ptr), void),                    \
                   "pointer type mismatch in container_of()");    \

I narrowed down the problem into the following test code:

  --------------------(test_code.c begin)--------------------
  struct foo {
          int (*callback)(void);
  };

  void assert(int);

  static inline struct foo *get_foo(void)
  {
          assert(__builtin_types_compatible_p(void, void));

          return (struct foo *)0;
  }

  int test(void);
  int test(void)
  {
          return get_foo()->callback();
  }
  ---------------------(test_code.c end)---------------------

Of course, GCC (and Clang as well) can compile it:

  $ gcc -Wall -c -o test_code.o test_code.c

However, Sparse complains about this obviously correct code:

  $ sparse test_code.c
  test_code.c:9:45: warning: unknown expression (4 0)
  test_code.c:9:51: warning: unknown expression (4 0)

Interstingly, just removing the 'inline' keyword in the test code
makes Sparse happy.

I concluded that Sparse cannot handle __builtin_types_compatible_p()
correctly. Make it no-op.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3:
  - New patch

Changes in v2: None

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

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 4a3f9c0..9e7da0b 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -23,6 +23,7 @@
 extern void __chk_user_ptr(const volatile void __user *);
 extern void __chk_io_ptr(const volatile void __iomem *);
 # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
+# define __builtin_types_compatible_p(t1, t2)	(1)
 #else /* __CHECKER__ */
 # ifdef STRUCTLEAK_PLUGIN
 #  define __user __attribute__((user))
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()
  2018-11-19 10:31 [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Masahiro Yamada
@ 2018-11-19 10:31 ` Masahiro Yamada
  2018-11-19 18:10   ` Nick Desaulniers
  2018-11-19 10:31 ` [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse Masahiro Yamada
  2018-11-19 12:33 ` [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Luc Van Oostenryck
  2 siblings, 1 reply; 9+ messages in thread
From: Masahiro Yamada @ 2018-11-19 10:31 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Luc Van Oostenryck, Nick Desaulniers, Kees Cook, Josh Triplett,
	Masahiro Yamada, linux-kernel

The kernel can only be compiled with an optimization option (-O2, -Os,
or the currently proposed -Og). Hence, __OPTIMIZE__ is always defined
in the kernel source.

The fallback for the -O0 case is just hypothetical and pointless.
Moreover, commit 0bb95f80a38f ("Makefile: Globally enable VLA warning")
enabled -Wvla warning. The use of variable length arrays is banned.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Kees Cook <keescook@chromium.org>
---

Changes in v3: None
Changes in v2: None

 include/linux/build_bug.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index 43d1fd5..d415c64 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -51,23 +51,9 @@
  * If you have some code which relies on certain constants being equal, or
  * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
  * detect if someone changes it.
- *
- * The implementation uses gcc's reluctance to create a negative array, but gcc
- * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
- * inline functions).  Luckily, in 4.3 they added the "error" function
- * attribute just for this type of case.  Thus, we use a negative sized array
- * (should always create an error on gcc versions older than 4.4) and then call
- * an undefined function with the error attribute (should always create an
- * error on gcc 4.3 and later).  If for some reason, neither creates a
- * compile-time error, we'll still have a link-time error, which is harder to
- * track down.
  */
-#ifndef __OPTIMIZE__
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-#else
 #define BUILD_BUG_ON(condition) \
 	BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
-#endif
 
 /**
  * BUILD_BUG - break compile if used.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse
  2018-11-19 10:31 [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Masahiro Yamada
  2018-11-19 10:31 ` [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON() Masahiro Yamada
@ 2018-11-19 10:31 ` Masahiro Yamada
  2018-11-19 12:37   ` Luc Van Oostenryck
  2018-11-19 12:33 ` [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Luc Van Oostenryck
  2 siblings, 1 reply; 9+ messages in thread
From: Masahiro Yamada @ 2018-11-19 10:31 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Luc Van Oostenryck, Nick Desaulniers, Kees Cook, Josh Triplett,
	Masahiro Yamada, linux-kernel

The introduction of these dummy BUILD_BUG_ON stubs dates back to
commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
for sparse").

At that time, BUILD_BUG_ON() was implemented with the negative array
trick *and* the link-time trick, like this:

  extern int __build_bug_on_failed;
  #define BUILD_BUG_ON(condition)                                \
          do {                                                   \
                  ((void)sizeof(char[1 - 2*!!(condition)]));     \
                  if (condition) __build_bug_on_failed = 1;      \
          } while(0)

Sparse is more strict about the negative array trick than GCC because
Sparse requires the array length to be really constant.

Here is the simple test code for the macro above:

  static const int x = 0;
  BUILD_BUG_ON(x);

GCC is absolutely fine with it (-Wvla was not enabled at that time),
but Sparse warns like this:

  error: bad constant expression
  error: cannot size expression

(If you are using a newer version of Sparse, you will see a different
warning message, "warning: Variable length array is used".)

Anyway, Sparse was producing many false positive warnings, hence
silenced.

With the previous commit, the leftover negative array trick is gone.
Sparse is fine with the current BUILD_BUG_ON(), which is implemented
by using the 'error' attribute. (assuming your Sparse version supports
-Wno-unknown-attribute option)

I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse
would complain about the following code, which GCC is fine with:

  static const int x = 0;
  int y = BUILD_BUG_ON_ZERO(x);

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Kees Cook <keescook@chromium.org>
---

Changes in v3:
 - Add Kees' Acked-by
 - Clarify log that BUILD_BUG_ON() *was* producing false positives
 - Keep the stub for BUILD_BUG_ON_ZERO()

Changes in v2:
 - Fix a coding style error (two consecutive blank lines)

 include/linux/build_bug.h | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index d415c64..faeec74 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -5,21 +5,8 @@
 #include <linux/compiler.h>
 
 #ifdef __CHECKER__
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_INVALID(e) (0)
-#define BUILD_BUG_ON_MSG(cond, msg) (0)
-#define BUILD_BUG_ON(condition) (0)
-#define BUILD_BUG() (0)
 #else /* __CHECKER__ */
-
-/* Force a compilation error if a constant expression is not a power of 2 */
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)	\
-	BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
-	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
-
 /*
  * Force a compilation error if condition is true, but also produce a
  * result (of value 0 and type size_t), so the expression can be used
@@ -27,6 +14,13 @@
  * aren't permitted).
  */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#endif /* __CHECKER__ */
+
+/* Force a compilation error if a constant expression is not a power of 2 */
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)	\
+	BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
+	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
@@ -64,6 +58,4 @@
  */
 #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
 
-#endif	/* __CHECKER__ */
-
 #endif	/* _LINUX_BUILD_BUG_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse
  2018-11-19 10:31 [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Masahiro Yamada
  2018-11-19 10:31 ` [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON() Masahiro Yamada
  2018-11-19 10:31 ` [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse Masahiro Yamada
@ 2018-11-19 12:33 ` Luc Van Oostenryck
  2018-11-20  1:32   ` Masahiro Yamada
  2 siblings, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2018-11-19 12:33 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Andrew Morton, Nick Desaulniers, Kees Cook, Josh Triplett,
	Miguel Ojeda, Arnd Bergmann, linux-kernel, Paul Burton

On Mon, Nov 19, 2018 at 07:31:41PM +0900, Masahiro Yamada wrote:
> When I tried to delete BUILD_BUG_ON stubs for sparse, the kbuild test
> robot reported lots of Sparse warnings from container_of(), which
> seem false positive.
> 
> The following checker in container_of() seems to be causing something
> strange for Sparse.
> 
>   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
>                    !__same_type(*(ptr), void),                    \
>                    "pointer type mismatch in container_of()");    \
> 
> I narrowed down the problem into the following test code:
> 
>   --------------------(test_code.c begin)--------------------
>   struct foo {
>           int (*callback)(void);
>   };
> 
>   void assert(int);
> 
>   static inline struct foo *get_foo(void)
>   {
>           assert(__builtin_types_compatible_p(void, void));
> 
>           return (struct foo *)0;
>   }
> 
>   int test(void);
>   int test(void)
>   {
>           return get_foo()->callback();
>   }
>   ---------------------(test_code.c end)---------------------
> 
> Of course, GCC (and Clang as well) can compile it:
> 
>   $ gcc -Wall -c -o test_code.o test_code.c
> 
> However, Sparse complains about this obviously correct code:
> 
>   $ sparse test_code.c
>   test_code.c:9:45: warning: unknown expression (4 0)
>   test_code.c:9:51: warning: unknown expression (4 0)
> 
> Interstingly, just removing the 'inline' keyword in the test code
> makes Sparse happy.
> 
> I concluded that Sparse cannot handle __builtin_types_compatible_p()
> correctly.

I think it's only caused by comparing 'void' (which is never
an l-value).
I'll investigate. Thanks for the small test-case.

> Make it no-op.

...

> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 4a3f9c0..9e7da0b 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -23,6 +23,7 @@
>  extern void __chk_user_ptr(const volatile void __user *);
>  extern void __chk_io_ptr(const volatile void __iomem *);
>  # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
> +# define __builtin_types_compatible_p(t1, t2)	(1)

Now, BUILD_BUG_ON() becomes a no-op for sparse but all the other usages
of __builtin_types_compatible_p() become potentially wrong and can now
create their onw false warnings.

Regards,
-- Luc

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse
  2018-11-19 10:31 ` [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse Masahiro Yamada
@ 2018-11-19 12:37   ` Luc Van Oostenryck
  2018-11-19 18:00     ` Nick Desaulniers
  0 siblings, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2018-11-19 12:37 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Andrew Morton, Nick Desaulniers, Kees Cook, Josh Triplett, linux-kernel

On Mon, Nov 19, 2018 at 07:31:43PM +0900, Masahiro Yamada wrote:
> The introduction of these dummy BUILD_BUG_ON stubs dates back to
> commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> for sparse").
> 
> At that time, BUILD_BUG_ON() was implemented with the negative array
> trick *and* the link-time trick, like this:
> 
>   extern int __build_bug_on_failed;
>   #define BUILD_BUG_ON(condition)                                \
>           do {                                                   \
>                   ((void)sizeof(char[1 - 2*!!(condition)]));     \
>                   if (condition) __build_bug_on_failed = 1;      \
>           } while(0)
> 
> Sparse is more strict about the negative array trick than GCC because
> Sparse requires the array length to be really constant.
> 
> Here is the simple test code for the macro above:
> 
>   static const int x = 0;
>   BUILD_BUG_ON(x);
> 
> GCC is absolutely fine with it (-Wvla was not enabled at that time),
> but Sparse warns like this:
> 
>   error: bad constant expression
>   error: cannot size expression
> 
> (If you are using a newer version of Sparse, you will see a different
> warning message, "warning: Variable length array is used".)
> 
> Anyway, Sparse was producing many false positive warnings, hence
> silenced.
> 
> With the previous commit, the leftover negative array trick is gone.
> Sparse is fine with the current BUILD_BUG_ON(), which is implemented
> by using the 'error' attribute. (assuming your Sparse version supports
> -Wno-unknown-attribute option)
> 
> I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse
> would complain about the following code, which GCC is fine with:
> 
>   static const int x = 0;
>   int y = BUILD_BUG_ON_ZERO(x);
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse
  2018-11-19 12:37   ` Luc Van Oostenryck
@ 2018-11-19 18:00     ` Nick Desaulniers
  2018-11-20  1:30       ` Masahiro Yamada
  0 siblings, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2018-11-19 18:00 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Masahiro Yamada, Andrew Morton, Kees Cook, josh, LKML

On Mon, Nov 19, 2018 at 4:37 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Mon, Nov 19, 2018 at 07:31:43PM +0900, Masahiro Yamada wrote:
> > The introduction of these dummy BUILD_BUG_ON stubs dates back to
> > commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> > for sparse").
> >
> > At that time, BUILD_BUG_ON() was implemented with the negative array
> > trick *and* the link-time trick, like this:
> >
> >   extern int __build_bug_on_failed;
> >   #define BUILD_BUG_ON(condition)                                \
> >           do {                                                   \
> >                   ((void)sizeof(char[1 - 2*!!(condition)]));     \
> >                   if (condition) __build_bug_on_failed = 1;      \
> >           } while(0)
> >
> > Sparse is more strict about the negative array trick than GCC because
> > Sparse requires the array length to be really constant.
> >
> > Here is the simple test code for the macro above:
> >
> >   static const int x = 0;
> >   BUILD_BUG_ON(x);
> >
> > GCC is absolutely fine with it (-Wvla was not enabled at that time),
> > but Sparse warns like this:
> >
> >   error: bad constant expression
> >   error: cannot size expression
> >
> > (If you are using a newer version of Sparse, you will see a different
> > warning message, "warning: Variable length array is used".)
> >
> > Anyway, Sparse was producing many false positive warnings, hence
> > silenced.
> >
> > With the previous commit, the leftover negative array trick is gone.
> > Sparse is fine with the current BUILD_BUG_ON(), which is implemented
> > by using the 'error' attribute. (assuming your Sparse version supports
> > -Wno-unknown-attribute option)
> >
> > I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse
> > would complain about the following code, which GCC is fine with:
> >
> >   static const int x = 0;
> >   int y = BUILD_BUG_ON_ZERO(x);
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Acked-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Clang builds not affected. Tested a quick arm64 defconfig build with
Clang + this patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()
  2018-11-19 10:31 ` [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON() Masahiro Yamada
@ 2018-11-19 18:10   ` Nick Desaulniers
  0 siblings, 0 replies; 9+ messages in thread
From: Nick Desaulniers @ 2018-11-19 18:10 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: Andrew Morton, Luc Van Oostenryck, Kees Cook, josh, LKML

On Mon, Nov 19, 2018 at 2:32 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> The kernel can only be compiled with an optimization option (-O2, -Os,
> or the currently proposed -Og). Hence, __OPTIMIZE__ is always defined
> in the kernel source.
>
> The fallback for the -O0 case is just hypothetical and pointless.
> Moreover, commit 0bb95f80a38f ("Makefile: Globally enable VLA warning")
> enabled -Wvla warning. The use of variable length arrays is banned.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  include/linux/build_bug.h | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
> index 43d1fd5..d415c64 100644
> --- a/include/linux/build_bug.h
> +++ b/include/linux/build_bug.h
> @@ -51,23 +51,9 @@
>   * If you have some code which relies on certain constants being equal, or
>   * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
>   * detect if someone changes it.
> - *
> - * The implementation uses gcc's reluctance to create a negative array, but gcc
> - * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
> - * inline functions).  Luckily, in 4.3 they added the "error" function
> - * attribute just for this type of case.  Thus, we use a negative sized array
> - * (should always create an error on gcc versions older than 4.4) and then call
> - * an undefined function with the error attribute (should always create an
> - * error on gcc 4.3 and later).  If for some reason, neither creates a
> - * compile-time error, we'll still have a link-time error, which is harder to
> - * track down.
>   */
> -#ifndef __OPTIMIZE__
> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> -#else
>  #define BUILD_BUG_ON(condition) \
>         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> -#endif
>
>  /**
>   * BUILD_BUG - break compile if used.
> --
> 2.7.4
>

Yep seems fine.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested an arm64 defconfig with Clang + this patch. Then tested again
with a `BUILD_BUG_ON(4 != 5)` to verify this still breaks the build.
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse
  2018-11-19 18:00     ` Nick Desaulniers
@ 2018-11-20  1:30       ` Masahiro Yamada
  0 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2018-11-20  1:30 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Luc Van Oostenryck, Andrew Morton, Kees Cook, Josh Triplett,
	Linux Kernel Mailing List

On Tue, Nov 20, 2018 at 3:02 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Nov 19, 2018 at 4:37 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > On Mon, Nov 19, 2018 at 07:31:43PM +0900, Masahiro Yamada wrote:
> > > The introduction of these dummy BUILD_BUG_ON stubs dates back to
> > > commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> > > for sparse").
> > >
> > > At that time, BUILD_BUG_ON() was implemented with the negative array
> > > trick *and* the link-time trick, like this:
> > >
> > >   extern int __build_bug_on_failed;
> > >   #define BUILD_BUG_ON(condition)                                \
> > >           do {                                                   \
> > >                   ((void)sizeof(char[1 - 2*!!(condition)]));     \
> > >                   if (condition) __build_bug_on_failed = 1;      \
> > >           } while(0)
> > >
> > > Sparse is more strict about the negative array trick than GCC because
> > > Sparse requires the array length to be really constant.
> > >
> > > Here is the simple test code for the macro above:
> > >
> > >   static const int x = 0;
> > >   BUILD_BUG_ON(x);
> > >
> > > GCC is absolutely fine with it (-Wvla was not enabled at that time),
> > > but Sparse warns like this:
> > >
> > >   error: bad constant expression
> > >   error: cannot size expression
> > >
> > > (If you are using a newer version of Sparse, you will see a different
> > > warning message, "warning: Variable length array is used".)
> > >
> > > Anyway, Sparse was producing many false positive warnings, hence
> > > silenced.
> > >
> > > With the previous commit, the leftover negative array trick is gone.
> > > Sparse is fine with the current BUILD_BUG_ON(), which is implemented
> > > by using the 'error' attribute. (assuming your Sparse version supports
> > > -Wno-unknown-attribute option)
> > >
> > > I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse
> > > would complain about the following code, which GCC is fine with:
> > >
> > >   static const int x = 0;
> > >   int y = BUILD_BUG_ON_ZERO(x);
> > >
> > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > Acked-by: Kees Cook <keescook@chromium.org>
> >
> > Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>
> Clang builds not affected. Tested a quick arm64 defconfig build with
> Clang + this patch.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>


This patch can go in only when 1/3 is acceptable.

But, I see 1/3 is controversial.



-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse
  2018-11-19 12:33 ` [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Luc Van Oostenryck
@ 2018-11-20  1:32   ` Masahiro Yamada
  0 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2018-11-20  1:32 UTC (permalink / raw)
  To: Luc Van Oostenryck
  Cc: Andrew Morton, Nick Desaulniers, Kees Cook, Josh Triplett,
	Miguel Ojeda, Arnd Bergmann, Linux Kernel Mailing List,
	Paul Burton

On Mon, Nov 19, 2018 at 9:35 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Mon, Nov 19, 2018 at 07:31:41PM +0900, Masahiro Yamada wrote:
> > When I tried to delete BUILD_BUG_ON stubs for sparse, the kbuild test
> > robot reported lots of Sparse warnings from container_of(), which
> > seem false positive.
> >
> > The following checker in container_of() seems to be causing something
> > strange for Sparse.
> >
> >   BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
> >                    !__same_type(*(ptr), void),                    \
> >                    "pointer type mismatch in container_of()");    \
> >
> > I narrowed down the problem into the following test code:
> >
> >   --------------------(test_code.c begin)--------------------
> >   struct foo {
> >           int (*callback)(void);
> >   };
> >
> >   void assert(int);
> >
> >   static inline struct foo *get_foo(void)
> >   {
> >           assert(__builtin_types_compatible_p(void, void));
> >
> >           return (struct foo *)0;
> >   }
> >
> >   int test(void);
> >   int test(void)
> >   {
> >           return get_foo()->callback();
> >   }
> >   ---------------------(test_code.c end)---------------------
> >
> > Of course, GCC (and Clang as well) can compile it:
> >
> >   $ gcc -Wall -c -o test_code.o test_code.c
> >
> > However, Sparse complains about this obviously correct code:
> >
> >   $ sparse test_code.c
> >   test_code.c:9:45: warning: unknown expression (4 0)
> >   test_code.c:9:51: warning: unknown expression (4 0)
> >
> > Interstingly, just removing the 'inline' keyword in the test code
> > makes Sparse happy.
> >
> > I concluded that Sparse cannot handle __builtin_types_compatible_p()
> > correctly.
>
> I think it's only caused by comparing 'void' (which is never
> an l-value).
> I'll investigate. Thanks for the small test-case.


Yes, please.


> > Make it no-op.
>
> ...
>
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 4a3f9c0..9e7da0b 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -23,6 +23,7 @@
> >  extern void __chk_user_ptr(const volatile void __user *);
> >  extern void __chk_io_ptr(const volatile void __iomem *);
> >  # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
> > +# define __builtin_types_compatible_p(t1, t2)        (1)
>
> Now, BUILD_BUG_ON() becomes a no-op for sparse but all the other usages
> of __builtin_types_compatible_p() become potentially wrong and can now
> create their onw false warnings.

You are right.
This patch is probably a bad idea.


Thanks.


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-11-20  1:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 10:31 [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Masahiro Yamada
2018-11-19 10:31 ` [PATCH v3 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON() Masahiro Yamada
2018-11-19 18:10   ` Nick Desaulniers
2018-11-19 10:31 ` [PATCH v3 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse Masahiro Yamada
2018-11-19 12:37   ` Luc Van Oostenryck
2018-11-19 18:00     ` Nick Desaulniers
2018-11-20  1:30       ` Masahiro Yamada
2018-11-19 12:33 ` [PATCH v3 1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse Luc Van Oostenryck
2018-11-20  1:32   ` Masahiro Yamada

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 NNTP newsgroup(s).