[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4546ce0f-8f29-4708-8af6-82fd1003e4bb@embeddedor.com>
Date: Mon, 1 Sep 2025 17:44:38 +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 17:21, Gustavo A. R. Silva wrote:
>
>
> 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;
Oh, a correction about this. Actually, if we need to use __packed, we would
have to pass it as an argument to TRAILING_OVERLAP(), like this:
-#define TRAILING_OVERLAP(TYPE, NAME, FAM, MEMBERS) \
+#define TRAILING_OVERLAP(TYPE, NAME, FAM, MEMBERS, ATTRS) \
union { \
TYPE NAME; \
struct { \
unsigned char __offset_to_##FAM[offsetof(TYPE, FAM)]; \
MEMBERS \
- }; \
+ } ATTRS; \
}
However, in this case MEMBERS is only cgrp_ancestor_storage, and it's correctly
aligned to __offset_to_##FAM[offsetof(TYPE, FAM)]; inside the helper. So, we
don't really need to pack that internal struct.
Thanks
-Gustavo
>
>
> 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
Powered by blists - more mailing lists