[PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Masahiro Yamada]

When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
reported lots of "unknown expression" warnings from container_of(),
which seemed false positive.

I addressed this in [1], but fixing Sparse is the right thing to do.

The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
function designator"), but it will take time until the fixed version
of Sparse is widely available.

Disable the container_of() type checks for Sparse for now.

[1] https://lore.kernel.org/lkml/1542623503-3755-1-git-send-email-yamada.masahiro@socionext.com/

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

Changes in v4:
  - New patch

Changes in v3: None
Changes in v2: None

 include/linux/kernel.h | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6aac75..d8c4adb 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -985,6 +985,21 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #define __CONCAT(a, b) a ## b
 #define CONCATENATE(a, b) __CONCAT(a, b)
 
+/*
+ * TODO:
+ * Sparse emits "unknown expression" warnings.
+ * It was fixed by commit 0eb8175d3e9c0d20354763d07ce3d4c0e543d988 in Sparse.
+ * Remove the following workaround when the fixed Sparse is widely available.
+ */
+#ifdef __CHECKER__
+#define TYPE_CHECK_CONTAINER_OF(ptr, type, member)	do {} while (0)
+#else
+#define TYPE_CHECK_CONTAINER_OF(ptr, type, member) \
+	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
+			 !__same_type(*(ptr), void),			\
+			 "pointer type mismatch in container_of()")
+#endif
+
 /**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
@@ -994,9 +1009,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  */
 #define container_of(ptr, type, member) ({				\
 	void *__mptr = (void *)(ptr);					\
-	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
-			 !__same_type(*(ptr), void),			\
-			 "pointer type mismatch in container_of()");	\
+	TYPE_CHECK_CONTAINER_OF(ptr, type, member);			\
 	((type *)(__mptr - offsetof(type, member))); })
 
 /**
@@ -1009,9 +1022,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  */
 #define container_of_safe(ptr, type, member) ({				\
 	void *__mptr = (void *)(ptr);					\
-	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
-			 !__same_type(*(ptr), void),			\
-			 "pointer type mismatch in container_of()");	\
+	TYPE_CHECK_CONTAINER_OF(ptr, type, member);			\
 	IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) :			\
 		((type *)(__mptr - offsetof(type, member))); })
 
-- 
2.7.4

[PATCH v4 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()

[Masahiro Yamada]

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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---

Changes in v4: None
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

Re: [PATCH v4 2/3] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()

[Miguel Ojeda]

On Thu, Nov 22, 2018 at 5:15 PM 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>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Didn't see v3/v4 (wasn't CC):

Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

Cheers,
Miguel

[PATCH v4 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for Sparse

[Masahiro Yamada]

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 enabled only very recently),
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 positives, and noisier than
it should be at that time.

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.

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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---

Changes in v4: None
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

Re: [PATCH v4 3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for Sparse

[Miguel Ojeda]

On Thu, Nov 22, 2018 at 5:08 PM Masahiro Yamada
<yamada.masahiro@socionext.com> 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 enabled only very recently),
> 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 positives, and noisier than
> it should be at that time.
>
> 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.
>
> 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>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Nice to see those CHECKER blocks are being reduced!

Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

Cheers,
Miguel

Re: [PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Andrew Morton]

On Thu, 22 Nov 2018 12:14:20 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
> reported lots of "unknown expression" warnings from container_of(),
> which seemed false positive.
> 
> I addressed this in [1], but fixing Sparse is the right thing to do.
> 
> The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
> function designator"), but it will take time until the fixed version
> of Sparse is widely available.
> 
> Disable the container_of() type checks for Sparse for now.
> 
> [1] https://lore.kernel.org/lkml/1542623503-3755-1-git-send-email-yamada.masahiro@socionext.com/
> 
> ...
>
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -985,6 +985,21 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  #define __CONCAT(a, b) a ## b
>  #define CONCATENATE(a, b) __CONCAT(a, b)
>  
> +/*
> + * TODO:
> + * Sparse emits "unknown expression" warnings.
> + * It was fixed by commit 0eb8175d3e9c0d20354763d07ce3d4c0e543d988 in Sparse.
> + * Remove the following workaround when the fixed Sparse is widely available.
> + */
> +#ifdef __CHECKER__
> +#define TYPE_CHECK_CONTAINER_OF(ptr, type, member)	do {} while (0)
> +#else
> +#define TYPE_CHECK_CONTAINER_OF(ptr, type, member) \
> +	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
> +			 !__same_type(*(ptr), void),			\
> +			 "pointer type mismatch in container_of()")
> +#endif

I think that's OK.  A few years hence, someone will happen upon this
comment and will perform the obvious cleanup.

Re: [PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Luc Van Oostenryck]

On Thu, Nov 22, 2018 at 12:14:20PM +0900, Masahiro Yamada wrote:
> When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
> reported lots of "unknown expression" warnings from container_of(),
> which seemed false positive.
> 
> I addressed this in [1], but fixing Sparse is the right thing to do.
> 
> The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
> function designator"), but it will take time until the fixed version
> of Sparse is widely available.
> 
> Disable the container_of() type checks for Sparse for now.

I would prefer that developers upgrade their version of sparse but ...

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

Re: [PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Miguel Ojeda]

On Fri, Nov 23, 2018 at 10:14 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Thu, Nov 22, 2018 at 12:14:20PM +0900, Masahiro Yamada wrote:
> > When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
> > reported lots of "unknown expression" warnings from container_of(),
> > which seemed false positive.
> >
> > I addressed this in [1], but fixing Sparse is the right thing to do.
> >
> > The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
> > function designator"), but it will take time until the fixed version
> > of Sparse is widely available.
> >
> > Disable the container_of() type checks for Sparse for now.
>
> I would prefer that developers upgrade their version of sparse but ...
>
> Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Indeed. If someone is writing code for the latest kernels, I think it
is reasonable to assume they are able to use the latest sparse too,
since it is not required for compilation anyway.

Cheers,
Miguel

Re: [PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Masahiro Yamada]

On Sat, Nov 24, 2018 at 6:06 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Nov 23, 2018 at 10:14 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > On Thu, Nov 22, 2018 at 12:14:20PM +0900, Masahiro Yamada wrote:
> > > When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
> > > reported lots of "unknown expression" warnings from container_of(),
> > > which seemed false positive.
> > >
> > > I addressed this in [1], but fixing Sparse is the right thing to do.
> > >
> > > The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
> > > function designator"), but it will take time until the fixed version
> > > of Sparse is widely available.
> > >
> > > Disable the container_of() type checks for Sparse for now.
> >
> > I would prefer that developers upgrade their version of sparse but ...
> >
> > Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>
> Indeed. If someone is writing code for the latest kernels, I think it
> is reasonable to assume they are able to use the latest sparse too,
> since it is not required for compilation anyway.


I am OK with either way.

I leave this to Andrew.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 1/3] kernel.h: disable type-checks in container_of() for Sparse

[Masahiro Yamada]

Hi Andrew,


On Sat, Nov 24, 2018 at 6:06 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Nov 23, 2018 at 10:14 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > On Thu, Nov 22, 2018 at 12:14:20PM +0900, Masahiro Yamada wrote:
> > > When I tried to enable BUILD_BUG_ON for Sparse, the kbuild test robot
> > > reported lots of "unknown expression" warnings from container_of(),
> > > which seemed false positive.
> > >
> > > I addressed this in [1], but fixing Sparse is the right thing to do.
> > >
> > > The issue was fixed by Sparse commit 0eb8175d3e9c ("fix expansion of
> > > function designator"), but it will take time until the fixed version
> > > of Sparse is widely available.
> > >
> > > Disable the container_of() type checks for Sparse for now.
> >
> > I would prefer that developers upgrade their version of sparse but ...
> >
> > Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>
> Indeed. If someone is writing code for the latest kernels, I think it
> is reasonable to assume they are able to use the latest sparse too,
> since it is not required for compilation anyway.



I was reconsidering about this.

I saw other Sparse warnings anyway
unless I use the state-of-the-art version of Sparse.


So, now I think Luc and Miguel were right.


Requiring the latest Sparse is the right solution.
We do not need to take care of old Sparse.


Andrew,
Could you drop this patch please?


-- 
Best Regards
Masahiro Yamada