[PATCH v4 0/2] kernel.h: container_of() pointer checking

[PATCH v4 0/2] kernel.h: container_of() pointer checking

[Ian Abbott]

[I screwed up an 'Acked-by:' line on v3.]

Patch 2 changes the container_of() macro to improve the compatibility
checking when the member has array type.  As a bonus (?), if the pointer
neither points to a type compatible with the member nor points to a type
compatible with void, compiler errors are produced instead of warnings.

Patch 1 is a prerequisite to avoid a lot of warnings when <linux/bug.h>
is included by <linux/kernel.h>.

1) asm-generic/bug.h: declare struct pt_regs; before function prototype
2) kernel.h: handle pointers to arrays better in container_of()

 include/asm-generic/bug.h | 1 +
 include/linux/kernel.h    | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

[PATCH v4 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Ian Abbott]

The declaration of `__warn()` has `struct pt_regs *regs` as one of its
parameters.  This can result in compiler warnings if `struct regs` is
not already declared.  Add an empty declaration of `struct pt_regs` to
avoid the warnings.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v3: Actually, there was no v1 or v2.  I called this v3 to match the
series.
v4: Corrected 'Acked-by:' line in patch description.
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d6f4aed479a1..87191357d303 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 /* used internally by panic.c */
 struct warn_args;
+struct pt_regs;
 
 void __warn(const char *file, int line, void *caller, unsigned taint,
 	    struct pt_regs *regs, struct warn_args *args);
-- 
2.11.0

[PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

If the first parameter of container_of() is a pointer to a
non-const-qualified array type (and the third parameter names a
non-const-qualified array member), the local variable __mptr will be
defined with a const-qualified array type.  In ISO C, these types are
incompatible.  They work as expected in GNU C, but some versions will
issue warnings.  For example, GCC 4.9 produces the warning
"initialization from incompatible pointer type".

Here is an example of where the problem occurs:

-------------------------------------------------------
 #include <linux/kernel.h>
 #include <linux/module.h>

MODULE_LICENSE("GPL");

struct st {
	int a;
	char b[16];
};

static int __init example_init(void) {
	struct st t = { .a = 101, .b = "hello" };
	char (*p)[16] = &t.b;
	struct st *x = container_of(p, struct st, b);
	printk(KERN_DEBUG "%p %p\n", (void *)&t, (void *)x);
	return 0;
}

static void __exit example_exit(void) {
}

module_init(example_init);
module_exit(example_exit);
-------------------------------------------------------

Building the module with gcc-4.9 results in these warnings (where '{m}'
is the module source and '{k}' is the kernel source):

-------------------------------------------------------
In file included from {m}/example.c:1:0:
{m}/example.c: In function ‘example_init’:
{k}/include/linux/kernel.h:854:48: warning: initialization from
incompatible pointer type
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                ^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
                 ^
{k}/include/linux/kernel.h:854:48: warning: (near initialization for
‘x’)
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                ^
{m}/example.c:14:17: note: in expansion of macro ‘container_of’
  struct st *x = container_of(p, struct st, b);
                 ^
-------------------------------------------------------

Replace the type checking performed by the macro to avoid these
warnings.  Make sure `*(ptr)` either has type compatible with the
member, or has type compatible with `void`, ignoring qualifiers.  Raise
compiler errors if this is not true.  This is stronger than the previous
behaviour, which only resulted in compiler warnings for a type mismatch.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexander Potapenko <glider@google.com>
---
v2: Rebased and altered description to provide an example of when the
compiler warnings occur.  v1 (from 2016-10-10) also modified a
'container_of_safe()' macro that never made it out of "linux-next".
v3: Added back some type checking at the suggestion of Michal
Nazarewicz with some helpful hints by Peter Zijlstra.
v4: No change.
---
 include/linux/kernel.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..f5dba37aaa60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -11,6 +11,7 @@
 #include <linux/log2.h>
 #include <linux/typecheck.h>
 #include <linux/printk.h>
+#include <linux/bug.h>
 #include <asm/byteorder.h>
 #include <uapi/linux/kernel.h>
 
@@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @member:	the name of the member within the struct.
  *
  */
-#define container_of(ptr, type, member) ({			\
-	const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
-	(type *)( (char *)__mptr - offsetof(type,member) );})
+#define 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()");	\
+	((type *)((char *)(ptr) - offsetof(type, member))); })
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

Re: [PATCH v4 1/2] asm-generic/bug.h: declare struct pt_regs; before function prototype

[Michal Nazarewicz]

On Tue, May 23 2017, Ian Abbott wrote:
> The declaration of `__warn()` has `struct pt_regs *regs` as one of its
> parameters.  This can result in compiler warnings if `struct regs` is
> not already declared.  Add an empty declaration of `struct pt_regs` to
> avoid the warnings.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
> v3: Actually, there was no v1 or v2.  I called this v3 to match the
> series.
> v4: Corrected 'Acked-by:' line in patch description.
> ---
>  include/asm-generic/bug.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index d6f4aed479a1..87191357d303 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -97,6 +97,7 @@ extern void warn_slowpath_null(const char *file, const int line);
>  
>  /* used internally by panic.c */
>  struct warn_args;
> +struct pt_regs;
>  
>  void __warn(const char *file, int line, void *caller, unsigned taint,
>  	    struct pt_regs *regs, struct warn_args *args);

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Michal Nazarewicz]

On Tue, May 23 2017, Ian Abbott wrote:
> If the first parameter of container_of() is a pointer to a
> non-const-qualified array type (and the third parameter names a
> non-const-qualified array member), the local variable __mptr will be
> defined with a const-qualified array type.  In ISO C, these types are
> incompatible.  They work as expected in GNU C, but some versions will
> issue warnings.  For example, GCC 4.9 produces the warning
> "initialization from incompatible pointer type".
>
> Here is an example of where the problem occurs:
>
> -------------------------------------------------------
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>
> MODULE_LICENSE("GPL");
>
> struct st {
> 	int a;
> 	char b[16];
> };
>
> static int __init example_init(void) {
> 	struct st t = { .a = 101, .b = "hello" };
> 	char (*p)[16] = &t.b;
> 	struct st *x = container_of(p, struct st, b);
> 	printk(KERN_DEBUG "%p %p\n", (void *)&t, (void *)x);
> 	return 0;
> }
>
> static void __exit example_exit(void) {
> }
>
> module_init(example_init);
> module_exit(example_exit);
> -------------------------------------------------------
>
> Building the module with gcc-4.9 results in these warnings (where '{m}'
> is the module source and '{k}' is the kernel source):
>
> -------------------------------------------------------
> In file included from {m}/example.c:1:0:
> {m}/example.c: In function ‘example_init’:
> {k}/include/linux/kernel.h:854:48: warning: initialization from
> incompatible pointer type
>   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>                                                 ^
> {m}/example.c:14:17: note: in expansion of macro ‘container_of’
>   struct st *x = container_of(p, struct st, b);
>                  ^
> {k}/include/linux/kernel.h:854:48: warning: (near initialization for
> ‘x’)
>   const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>                                                 ^
> {m}/example.c:14:17: note: in expansion of macro ‘container_of’
>   struct st *x = container_of(p, struct st, b);
>                  ^
> -------------------------------------------------------
>
> Replace the type checking performed by the macro to avoid these
> warnings.  Make sure `*(ptr)` either has type compatible with the
> member, or has type compatible with `void`, ignoring qualifiers.  Raise
> compiler errors if this is not true.  This is stronger than the previous
> behaviour, which only resulted in compiler warnings for a type mismatch.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Alexander Potapenko <glider@google.com>
> ---
> v2: Rebased and altered description to provide an example of when the
> compiler warnings occur.  v1 (from 2016-10-10) also modified a
> 'container_of_safe()' macro that never made it out of "linux-next".
> v3: Added back some type checking at the suggestion of Michal
> Nazarewicz with some helpful hints by Peter Zijlstra.
> v4: No change.
> ---
>  include/linux/kernel.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 13bc08aba704..f5dba37aaa60 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -11,6 +11,7 @@
>  #include <linux/log2.h>
>  #include <linux/typecheck.h>
>  #include <linux/printk.h>
> +#include <linux/bug.h>
>  #include <asm/byteorder.h>
>  #include <uapi/linux/kernel.h>
>  
> @@ -850,9 +851,11 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>   * @member:	the name of the member within the struct.
>   *
>   */
> -#define container_of(ptr, type, member) ({			\
> -	const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
> -	(type *)( (char *)__mptr - offsetof(type,member) );})
> +#define 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()");	\
> +	((type *)((char *)(ptr) - offsetof(type, member))); })
>  
>  /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

Hi Ian,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ian-Abbott/asm-generic-bug-h-declare-struct-pt_regs-before-function-prototype/20170524-070310
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:14:0,
                    from include/asm-generic/bug.h:15,
                    from arch/x86/include/asm/bug.h:81,
                    from drivers/block/drbd/drbd_interval.c:1:
>> include/linux/bug.h:103:47: warning: 'struct bug_entry' declared inside parameter list will not be visible outside of this definition or declaration
    static inline int is_warning_bug(const struct bug_entry *bug)
                                                  ^~~~~~~~~
   include/linux/bug.h: In function 'is_warning_bug':
>> include/linux/bug.h:105:12: error: dereferencing pointer to incomplete type 'const struct bug_entry'
     return bug->flags & BUGFLAG_WARNING;
               ^~

vim +105 include/linux/bug.h

ff20c2e0 Kirill A. Shutemov  2016-03-01   97  
35edd910 Paul Gortmaker      2011-11-16   98  #endif	/* __CHECKER__ */
35edd910 Paul Gortmaker      2011-11-16   99  
7664c5a1 Jeremy Fitzhardinge 2006-12-08  100  #ifdef CONFIG_GENERIC_BUG
7664c5a1 Jeremy Fitzhardinge 2006-12-08  101  #include <asm-generic/bug.h>
7664c5a1 Jeremy Fitzhardinge 2006-12-08  102  
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @103  static inline int is_warning_bug(const struct bug_entry *bug)
7664c5a1 Jeremy Fitzhardinge 2006-12-08  104  {
7664c5a1 Jeremy Fitzhardinge 2006-12-08 @105  	return bug->flags & BUGFLAG_WARNING;
7664c5a1 Jeremy Fitzhardinge 2006-12-08  106  }
7664c5a1 Jeremy Fitzhardinge 2006-12-08  107  
19d43626 Peter Zijlstra      2017-02-25  108  struct bug_entry *find_bug(unsigned long bugaddr);

:::::: The code at line 105 was first introduced by commit
:::::: 7664c5a1da4711bb6383117f51b94c8dc8f3f1cd [PATCH] Generic BUG implementation

:::::: TO: Jeremy Fitzhardinge <jeremy@goop.org>
:::::: CC: Linus Torvalds <torvalds@woody.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60103 bytes --]

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 24/05/17 01:54, kbuild test robot wrote:
> Hi Ian,
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.12-rc2 next-20170523]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Ian-Abbott/asm-generic-bug-h-declare-struct-pt_regs-before-function-prototype/20170524-070310
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All error/warnings (new ones prefixed by >>):
>
>    In file included from include/linux/kernel.h:14:0,
>                     from include/asm-generic/bug.h:15,
>                     from arch/x86/include/asm/bug.h:81,
>                     from drivers/block/drbd/drbd_interval.c:1:
>>> include/linux/bug.h:103:47: warning: 'struct bug_entry' declared inside parameter list will not be visible outside of this definition or declaration
>     static inline int is_warning_bug(const struct bug_entry *bug)
>                                                   ^~~~~~~~~
>    include/linux/bug.h: In function 'is_warning_bug':
>>> include/linux/bug.h:105:12: error: dereferencing pointer to incomplete type 'const struct bug_entry'
>      return bug->flags & BUGFLAG_WARNING;
>                ^~
>
> vim +105 include/linux/bug.h
>
> ff20c2e0 Kirill A. Shutemov  2016-03-01   97
> 35edd910 Paul Gortmaker      2011-11-16   98  #endif	/* __CHECKER__ */
> 35edd910 Paul Gortmaker      2011-11-16   99
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  100  #ifdef CONFIG_GENERIC_BUG
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  101  #include <asm-generic/bug.h>
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  102
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08 @103  static inline int is_warning_bug(const struct bug_entry *bug)
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  104  {
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08 @105  	return bug->flags & BUGFLAG_WARNING;
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  106  }
> 7664c5a1 Jeremy Fitzhardinge 2006-12-08  107
> 19d43626 Peter Zijlstra      2017-02-25  108  struct bug_entry *find_bug(unsigned long bugaddr);
>
> :::::: The code at line 105 was first introduced by commit
> :::::: 7664c5a1da4711bb6383117f51b94c8dc8f3f1cd [PATCH] Generic BUG implementation
>
> :::::: TO: Jeremy Fitzhardinge <jeremy@goop.org>
> :::::: CC: Linus Torvalds <torvalds@woody.osdl.org>

This breakage occurs when <asm-generic/bug.h> is included before 
<include/linux.h> due to a circular dependancy I introduced with this 
commit.

It can be fixed by replacing the `#include <linux/kernel.h>` with 
`#include <linux/types.h>` in "include/asm-generic/bug.h".  I can send a 
patch to fix that, but ideally, it should be committed before this 
commit to avoid breakage during git bisect builds.  So my question is, 
should I send a single patch to fix this breakage, or resend the series 
of patches to incorporate this fix before the current commit?

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH v4 2/2] kernel.h: handle pointers to arrays better in container_of()

[Andrew Morton]

On Wed, 24 May 2017 11:46:17 +0100 Ian Abbott <abbotti@mev.co.uk> wrote:

> This breakage occurs when <asm-generic/bug.h> is included before 
> <include/linux.h> due to a circular dependancy I introduced with this 
> commit.
> 
> It can be fixed by replacing the `#include <linux/kernel.h>` with 
> `#include <linux/types.h>` in "include/asm-generic/bug.h".  I can send a 
> patch to fix that, but ideally, it should be committed before this 
> commit to avoid breakage during git bisect builds.  So my question is, 
> should I send a single patch to fix this breakage, or resend the series 
> of patches to incorporate this fix before the current commit?

I expect a full resend would be best - that way the test robot should
be able to parse and retest it in the usual fashion.