linux-kernel - Re: [RFC] cgroup: Avoid thousands of -Wflex-array-member-not-at-end warnings

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <d0c49dc9-c810-47d2-a3ce-d74196a39235@embeddedor.com>
Date: Mon, 1 Sep 2025 17:21:22 +0200
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Michal Koutný <mkoutny@...e.com>
Cc: Tejun Heo <tj@...nel.org>, Johannes Weiner <hannes@...xchg.org>,
 cgroups@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
 linux-hardening@...r.kernel.org, "Gustavo A. R. Silva"
 <gustavoars@...nel.org>, Chen Ridong <chenridong@...weicloud.com>
Subject: Re: [RFC] cgroup: Avoid thousands of -Wflex-array-member-not-at-end
 warnings

On 9/1/25 15:37, Michal Koutný wrote:
> On Sat, Aug 30, 2025 at 03:30:11PM +0200, "Gustavo A. R. Silva" <gustavo@...eddedor.com> wrote:
>> Based on the comments above, it seems that the original code was expecting
>> cgrp->ancestors[0] and cgrp_ancestor_storage to share the same addres in
>> memory.
> 
> Fortunately, it doesn't matter what the address of cgrp_ancestor_storage
> is. The important effect is that cgroup_root::cgrp is followed by
> sufficient space to store a pointer (accessed via cgroup::ancestors[0]).
> 
>> However when I take a look at the pahole output, I see that these two members
>> are actually misaligned by 56 bytes. See below:
> 
> So the root cgroup's ancestry may be saved inside the padding instead of
> the dedicated storage. I don't think it causes immediate issues but it'd
> be better not to write to these bytes. (Note that the layout depends on
> kernel config.) Thanks for the report Gustavo!
> 
> 
>> So, one solution for this is to use the TRAILING_OVERLAP() helper and
>> move these members at the end of `struct cgroup_root`. With this the
>> misalignment disappears (together with the 14722 warnings :) ), and now
>> both cgrp->ancestors[0] and cgrp_ancestor_storage share the same address
>> in memory. See below:
> 
> I didn't know TRAILING_OVERLAP() but it sounds like the tool for such
> situations.

I recently introduced it. :)

> Why do you move struct cgroup at the end of struct cgroup_root?

Because struct cgroup ends in a flexible-array member `ancestors`.
This triggers the -Wflex-array-member-not-at-end warns about. So,
while `ancestors` is indeed a flexible array, any instance of
cgroup embedded in another struct should be placed at the end.

However, if we change it to something like this (and of course
updating any related code, accordingly):

-       struct cgroup *ancestors[];
+       struct cgroup **ancestors;

Then the flex in the middle issue goes away, and we can have
struct cgroup embedded in another struct anywhere.

The question is if this would be an acceptable solution?

I'd probably prefer this to remain a flexible-array member,
but I'd like to hear people's opinions and feedback. :)

> 
> (Actually, as I look at the macro's implementation, it should be
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -110,7 +110,7 @@ enum {
>                  struct {                                                        \
>                          unsigned char __offset_to_##FAM[offsetof(TYPE, FAM)];   \
>                          MEMBERS                                                 \
> -               };                                                              \
> +               } __packed;                                                     \
>          }
> 
>   #endif
> in order to avoid similar issues, no?)

The way to do it is like this:

+       TRAILING_OVERLAP(struct cgroup, cgrp, ancestors,
+               /* must follow cgrp for cgrp->ancestors[0], see above */
+               struct cgroup *cgrp_ancestor_storage;
+       ) __packed;

and this get us to the following:

struct cgroup_root {
...
         /* --- cacheline 65 boundary (4160 bytes) was 56 bytes ago --- */
         union {
                 struct cgroup      cgrp __attribute__((__aligned__(1))); /*  4216  8192 */
                 struct {
                         unsigned char __offset_to_ancestors[5784]; /*  4216  5784 */
                         /* --- cacheline 156 boundary (9984 bytes) was 16 bytes ago --- */
                         struct cgroup * cgrp_ancestor_storage; /* 10000     8 */
                 };                                       /*  4216  5792 */
         } __attribute__((__aligned__(1)));               /*  4216  8192 */

         /* size: 12408, cachelines: 194, members: 10 */
         /* forced alignments: 2 */
         /* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

Notice the change in alignment in struct cgroup_root from
4096 (page alignment) to 8 bytes alignment. In any case,
the size (still) increases from 6400 to 12408 bytes.

Thanks
-Gustavo