Re: [PATCH v2] kernel.h: handle pointers to arrays better in container_of()
From: Ian Abbott
Date: Tue May 23 2017 - 06:32:09 EST
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@xxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
Cc: Hidehiro Kawai <hidehiro.kawai.ez@xxxxxxxxxxx>
Cc: Borislav Petkov <bp@xxxxxxx>
Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
Cc: Johannes Berg <johannes.berg@xxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Alexander Potapenko <glider@xxxxxxxxxx>
---
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@xxxxxxxxx> )=-
-=( Web: http://www.mev.co.uk/ )=-