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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <363e7b83-64b4-6149-3378-ac28cdfd89f3@mev.co.uk>
Date:   Tue, 23 May 2017 15:14:03 +0100
From:   Ian Abbott <abbotti@....co.uk>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Michal Nazarewicz <mina86@...a86.com>,
        linux-kernel@...r.kernel.org,
        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>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v2] kernel.h: handle pointers to arrays better in
 container_of()

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@....co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-