linux-kernel - Re: [PATCH v2] kernel.h: handle pointers to arrays better in container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <139f0546-9b0a-d119-6b96-a9b35559be4b@mev.co.uk>
Date:   Tue, 23 May 2017 11:32:02 +0100
From:   Ian Abbott <abbotti@....co.uk>
To:     Michal Nazarewicz <mina86@...a86.com>, linux-kernel@...r.kernel.org
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
        Borislav Petkov <bp@...e.de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Johannes Berg <johannes.berg@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v2] kernel.h: handle pointers to arrays better in
 container_of()

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@....co.uk>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Michal Nazarewicz <mina86@...a86.com>
>> Cc: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
>> Cc: Borislav Petkov <bp@...e.de>
>> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
>> Cc: Johannes Berg <johannes.berg@...el.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Alexander Potapenko <glider@...gle.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@....co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-