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

[PATCH v2] 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);
                 ^
-------------------------------------------------------

Fix it by avoiding defining the __mptr variable.  This also avoids other
GCC extensions.

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".
---
 include/linux/kernel.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..169fe6f51b7b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -850,9 +850,8 @@ 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) \
+	((type *)((char *)(ptr) - offsetof(type, member)))
 
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
-- 
2.11.0

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

[Michal Nazarewicz]

On Mon, May 22 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);
>                  ^
> -------------------------------------------------------
>
> Fix it by avoiding defining the __mptr variable.  This also avoids other
> GCC extensions.
>
> 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".
> ---
>  include/linux/kernel.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 13bc08aba704..169fe6f51b7b 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -850,9 +850,8 @@ 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) \
> +	((type *)((char *)(ptr) - offsetof(type, member)))

Now the type of ptr is not checked though.  Using your example, I can
now write:

	struct st t = { .a = 101, .b = "hello" };
	int *p = &t.a;
	struct st *x = container_of(p, struct st, b);

and it will compile with no warnings.  Previously it would fail.  The
best I can think of would be (not tested):

#define container_of(ptr, type, member) (				\
	_Static_assert(__builtin_types_compatible_p(			\
            typeof(ptr), typeof( ((type *)0)->member )*), "WUT"),	\
	((type *)((char *)(ptr) - offsetof(type, member)));		\
)

or maybe:

#define container_of(ptr, type, member) (				\
	_Static_assert(__builtin_types_compatible_p(			\
            typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),	\
	((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 v2] kernel.h: handle pointers to arrays better in container_of()

[Ian Abbott]

On 22/05/17 18:58, Michal Nazarewicz wrote:
> On Mon, May 22 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);
>>                  ^
>> -------------------------------------------------------
>>
>> Fix it by avoiding defining the __mptr variable.  This also avoids other
>> GCC extensions.
>>
>> 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".
>> ---
>>  include/linux/kernel.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 13bc08aba704..169fe6f51b7b 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -850,9 +850,8 @@ 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) \
>> +	((type *)((char *)(ptr) - offsetof(type, member)))
>
> Now the type of ptr is not checked though.  Using your example, I can
> now write:
>
> 	struct st t = { .a = 101, .b = "hello" };
> 	int *p = &t.a;
> 	struct st *x = container_of(p, struct st, b);
>
> and it will compile with no warnings.  Previously it would fail.  The
> best I can think of would be (not tested):
>
> #define container_of(ptr, type, member) (				\
> 	_Static_assert(__builtin_types_compatible_p(			\
>             typeof(ptr), typeof( ((type *)0)->member )*), "WUT"),	\
> 	((type *)((char *)(ptr) - offsetof(type, member)));		\
> )
>
> or maybe:
>
> #define container_of(ptr, type, member) (				\
> 	_Static_assert(__builtin_types_compatible_p(			\
>             typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),	\
> 	((type *)((char *)(ptr) - offsetof(type, member)));		\
> )

It's a fine suggestion (if more parentheses are added), but 
_Static_assert is a C11 feature, and I thought the kernel was using 
gnu89 (unless it's been updated since).

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

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

[Peter Zijlstra]

On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:

> > #define container_of(ptr, type, member) (				\
> > 	_Static_assert(__builtin_types_compatible_p(			\
> >             typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),	\
> > 	((type *)((char *)(ptr) - offsetof(type, member)));		\
> > )
> 
> It's a fine suggestion (if more parentheses are added), but _Static_assert
> is a C11 feature, and I thought the kernel was using gnu89 (unless it's been
> updated since).

We have BUILD_BUG_ON() that should be similar in functionality.

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

[Ian Abbott]

On 23/05/17 12:24, Peter Zijlstra wrote:
> On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:
>
>>> #define container_of(ptr, type, member) (				\
>>> 	_Static_assert(__builtin_types_compatible_p(			\
>>>             typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),	\
>>> 	((type *)((char *)(ptr) - offsetof(type, member)));		\
>>> )
>>
>> It's a fine suggestion (if more parentheses are added), but _Static_assert
>> is a C11 feature, and I thought the kernel was using gnu89 (unless it's been
>> updated since).
>
> We have BUILD_BUG_ON() that should be similar in functionality.

Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid 
some baggage.  Also, __same_type() can be used for type checking.

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

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

[Ian Abbott]

On 23/05/17 13:02, Ian Abbott wrote:
> On 23/05/17 12:24, Peter Zijlstra wrote:
>> On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:
>>
>>>> #define container_of(ptr, type, member) (                \
>>>>     _Static_assert(__builtin_types_compatible_p(            \
>>>>             typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),    \
>>>>     ((type *)((char *)(ptr) - offsetof(type, member)));        \
>>>> )
>>>
>>> It's a fine suggestion (if more parentheses are added), but
>>> _Static_assert
>>> is a C11 feature, and I thought the kernel was using gnu89 (unless
>>> it's been
>>> updated since).
>>
>> We have BUILD_BUG_ON() that should be similar in functionality.
>
> Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
> some baggage.  Also, __same_type() can be used for type checking.
>

BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced 
lots of warnings about it having no effect, so I tried using 
BUILD_BUG_ON() instead as follows:

#define container_of(ptr, type, member) ({			\
	BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
	((type *)((char *)(ptr) - offsetof(type, member)));})

Unfortunately, this does result in compiler failures in places where the 
types do not in fact match, for example `slab_show()` in 
"mm/slab_common.c" (`list_entry()` uses `container_of()`):

static int slab_show(struct seq_file *m, void *p)
{
	struct kmem_cache *s = list_entry(p, struct kmem_cache, root_caches_node);


I then changed it to allow `ptr` to be a `void *` too:

#define container_of(ptr, type, member) ({			\
	BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
		     !__same_type((ptr), void *));		\
	((type *)((char *)(ptr) - offsetof(type, member)));})

Now it fails on `external_name()` in "fs/dcache.c":

static inline struct external_name *external_name(struct dentry *dentry)
{
	return container_of(dentry->d_name.name, struct external_name, name[0]);
}

Here, `dentry->d_name.name` is of type `const unsigned char *` and 
`name[0]` in `struct external_name` is of type `unsigned char`.  The 
error is:

error: call to ‘__compiletime_assert_258’ declared with attribute error: 
BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct 
external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)
   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                       ^

I'm not quite sure why that is failing yet.

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

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

[Ian Abbott]

On 23/05/17 14:23, Ian Abbott wrote:
> On 23/05/17 13:02, Ian Abbott wrote:
>> On 23/05/17 12:24, Peter Zijlstra wrote:
>>> On Tue, May 23, 2017 at 11:32:02AM +0100, Ian Abbott wrote:
>>>
>>>>> #define container_of(ptr, type, member) (                \
>>>>>     _Static_assert(__builtin_types_compatible_p(            \
>>>>>             typeof(*ptr), typeof( ((type *)0)->member )), "WUT"),    \
>>>>>     ((type *)((char *)(ptr) - offsetof(type, member)));        \
>>>>> )
>>>>
>>>> It's a fine suggestion (if more parentheses are added), but
>>>> _Static_assert
>>>> is a C11 feature, and I thought the kernel was using gnu89 (unless
>>>> it's been
>>>> updated since).
>>>
>>> We have BUILD_BUG_ON() that should be similar in functionality.
>>
>> Yes, I think BUILD_BUG_ON_ZERO() will be better in this case to avoid
>> some baggage.  Also, __same_type() can be used for type checking.
>>
>
> BUILD_BUG_ON_ZERO() in the left-hand side of a comma expression produced
> lots of warnings about it having no effect, so I tried using
> BUILD_BUG_ON() instead as follows:
>
> #define container_of(ptr, type, member) ({            \
>     BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member));\
>     ((type *)((char *)(ptr) - offsetof(type, member)));})
>
> Unfortunately, this does result in compiler failures in places where the
> types do not in fact match, for example `slab_show()` in
> "mm/slab_common.c" (`list_entry()` uses `container_of()`):
>
> static int slab_show(struct seq_file *m, void *p)
> {
>     struct kmem_cache *s = list_entry(p, struct kmem_cache,
> root_caches_node);
>
>
> I then changed it to allow `ptr` to be a `void *` too:
>
> #define container_of(ptr, type, member) ({            \
>     BUILD_BUG_ON(!__same_type((ptr), &((type *)0)->member) &&\
>              !__same_type((ptr), void *));        \
>     ((type *)((char *)(ptr) - offsetof(type, member)));})
>
> Now it fails on `external_name()` in "fs/dcache.c":
>
> static inline struct external_name *external_name(struct dentry *dentry)
> {
>     return container_of(dentry->d_name.name, struct external_name,
> name[0]);
> }
>
> Here, `dentry->d_name.name` is of type `const unsigned char *` and
> `name[0]` in `struct external_name` is of type `unsigned char`.  The
> error is:
>
> error: call to ‘__compiletime_assert_258’ declared with attribute error:
> BUILD_BUG_ON failed: !__same_type((dentry->d_name.name), &((struct
> external_name *)0)->name[0]) && !__same_type((dentry->d_name.name), void *)
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>                                       ^
>
> I'm not quite sure why that is failing yet.

It fails because `const unsigned char *` is not compatible with 
`unsigned char *` as fast as `__builtin_types_compatible_p()` is 
concerned (quite rightly so).  However, a simple change fixes that:

#define container_of(ptr, type, member) ({				\
	BUILD_BUG_ON(!__same_type(*(ptr), ((type *)0)->member) &&	\
		     !__same_type((ptr), void *));			\
	((type *)((char *)(ptr) - offsetof(type, member)));})

That one seems to work fine (after including <linux/bug.h> in kernel.h 
and fixing up include/asm-generic/bug.h a little bit, which will be in a 
separate patch.

Thanks for the help guys.  v3 will be a series of 2 patches.

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