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: <92912540-23d2-4b18-9002-bac962682caf@embeddedor.com>
Date: Tue, 2 Sep 2025 09:56:34 +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 19:58, Michal Koutný wrote:
> On Mon, Sep 01, 2025 at 05:21:22PM +0200, "Gustavo A. R. Silva" <gustavo@...eddedor.com> wrote:
>> 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.
> 
> Oh, so TRAILING_OVERLAP() won't work like that?
> (I thought that it'd hide the FAM from the end of the union and thus it
> could embedded when wrapped like this. On second thought, I realize
> that's exclusive with the static validations.)
> 
>> 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. :)
> 
> I'd prefer if cgroup_create could still work with one allocation only
> both for struct cgroup and its ancestors array. (Cgroup allocation
> happens many times in a day.)
> 
> The increase in struct cgroup_root size is IMO not that problematic.
> (There are typically at most CGROUP_SUBSYS_COUNT roots with gradual
> trend to only the single cgrp_dfl_root.)
> 
> Note that it'd be good to keep it enclosed within struct cgroup_root
> (cgroup1_root_to_use could use struct_size()), however, the
> cgrp_dfl_root would still need the storage somewhere.

If the increase in size is not a problem, then something like this
works fine (unless there is a problem with moving those two members
at the end of cgroup_root?):

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 539c64eeef38..bd28d639a78a 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -630,16 +630,6 @@ struct cgroup_root {
         struct list_head root_list;
         struct rcu_head rcu;    /* Must be near the top */

-       /*
-        * The root cgroup. The containing cgroup_root will be destroyed on its
-        * release. cgrp->ancestors[0] will be used overflowing into the
-        * following field. cgrp_ancestor_storage must immediately follow.
-        */
-       struct cgroup cgrp;
-
-       /* must follow cgrp for cgrp->ancestors[0], see above */
-       struct cgroup *cgrp_ancestor_storage;
-
         /* Number of cgroups in the hierarchy, used only for /proc/cgroups */
         atomic_t nr_cgrps;

@@ -651,7 +641,21 @@ struct cgroup_root {

         /* The name for this hierarchy - may be empty */
         char name[MAX_CGROUP_ROOT_NAMELEN];
+
+       /*
+        * The root cgroup. The containing cgroup_root will be destroyed on its
+        * release. cgrp->ancestors[0] will be used overflowing into the
+        * following field. cgrp_ancestor_storage must immediately follow.
+        *
+        * Must be last --ends in a flexible-array member.
+        */
+       TRAILING_OVERLAP(struct cgroup, cgrp, ancestors,
+               /* must follow cgrp for cgrp->ancestors[0], see above */
+               struct cgroup *cgrp_ancestor_storage;
+       );
  };
+static_assert(offsetof(struct cgroup_root, cgrp.ancestors) ==
+             offsetof(struct cgroup_root, cgrp_ancestor_storage));


The assert above checks that no misalignment is inadvertently
introduced between FAM and cgrp_ancestor_storage.

Thanks
-Gustavo