Message-ID: <50a3fe91-2120-4cd3-a64f-39d50ba9138c@embeddedor.com>
Date: Wed, 7 May 2025 11:41:55 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Jan Kara <jack@...e.cz>, Amir Goldstein <amir73il@...il.com>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Matthew Bobrowski <repnop@...gle.com>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] fanotify: Avoid a couple of
 -Wflex-array-member-not-at-end warnings



On 07/05/25 05:08, Jan Kara wrote:
> On Wed 07-05-25 07:56:21, Amir Goldstein wrote:
>> On Wed, May 7, 2025 at 1:39 AM Gustavo A. R. Silva
>> <gustavoars@...nel.org> wrote:
>>>
>>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>>> getting ready to enable it, globally.
>>>
>>> Modify FANOTIFY_INLINE_FH() macro, which defines a struct containing a
>>> flexible-array member in the middle (struct fanotify_fh::buf), to use
>>> struct_size_t() to pre-allocate space for both struct fanotify_fh and
>>> its flexible-array member. Replace the struct with a union and relocate
>>> the flexible structure (struct fanotify_fh) to the end.
>>>
>>> See the memory layout of struct fanotify_fid_event before and after
>>> changes below.
>>>
>>> pahole -C fanotify_fid_event fs/notify/fanotify/fanotify.o
>>>
>>> BEFORE:
>>> struct fanotify_fid_event {
>>>          struct fanotify_event      fae;                  /*     0    48 */
>>>          __kernel_fsid_t            fsid;                 /*    48     8 */
>>>          struct {
>>>                  struct fanotify_fh object_fh;            /*    56     4 */
>>>                  unsigned char      _inline_fh_buf[12];   /*    60    12 */
>>>          };                                               /*    56    16 */
>>>
>>>          /* size: 72, cachelines: 2, members: 3 */
>>>          /* last cacheline: 8 bytes */
>>> };
>>>
>>> AFTER:
>>> struct fanotify_fid_event {
>>>          struct fanotify_event      fae;                  /*     0    48 */
>>>          __kernel_fsid_t            fsid;                 /*    48     8 */
>>>          union {
>>>                  unsigned char      _inline_fh_buf[16];   /*    56    16 */
>>>                  struct fanotify_fh object_fh __attribute__((__aligned__(1))); /*    56     4 */
>>
>> I'm not that familiar with pahole, but I find it surprising to see this member
>> aligned(1), when struct fanotify_fh is defined as __aligned(4).
> 
> Yeah.

Yep, gotcha.

> 
>>>          } __attribute__((__aligned__(1)));               /*    56    16 */
>>>
>>>          /* size: 72, cachelines: 2, members: 3 */
>>>          /* forced alignments: 1 */
>>>          /* last cacheline: 8 bytes */
>>> } __attribute__((__aligned__(8)));
>>>
>>> So, with these changes, fix the following warnings:
>>>
>>> fs/notify/fanotify/fanotify.h:317:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>>> fs/notify/fanotify/fanotify.h:289:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>>>
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>>> ---
>>>   fs/notify/fanotify/fanotify.h | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
>>> index b44e70e44be6..91c26b1c1d32 100644
>>> --- a/fs/notify/fanotify/fanotify.h
>>> +++ b/fs/notify/fanotify/fanotify.h
>>> @@ -275,12 +275,12 @@ static inline void fanotify_init_event(struct fanotify_event *event,
>>>          event->pid = NULL;
>>>   }
>>>
>>> -#define FANOTIFY_INLINE_FH(name, size)                                 \
>>> -struct {                                                               \
>>> -       struct fanotify_fh name;                                        \
>>> -       /* Space for object_fh.buf[] - access with fanotify_fh_buf() */ \
>>> -       unsigned char _inline_fh_buf[size];                             \
>>> -}
>>> +#define FANOTIFY_INLINE_FH(name, size)                                               \
>>> +union {                                                                                      \
>>> +       /* Space for object_fh and object_fh.buf[] - access with fanotify_fh_buf() */ \
>>> +       unsigned char _inline_fh_buf[struct_size_t(struct fanotify_fh, buf, size)];   \
>>
>> The name _inline_fh_buf is confusing in this setting
>> better use bytes[] as in DEFINE_FLEX() or maybe even consider
>> a generic helper DEFINE_FLEX_MEMBER() to use instead of
>> FANOTIFY_INLINE_FH(), because this is not fanotify specific,
>> except maybe for alignment (see below).
> 
> Yes, I guess a generic helper for this would be nice but if fanotify is the
> only place that plays these tricks, we can keep it specific for now. I
> agree naming the "space-buffer" field "bytes" would be less confusing.

I can send v2 with this change.

> 
>>
>>> +       struct fanotify_fh name;                                                      \
>>> +} __packed
>>
>> Why added __packed?
>>
>> The fact that struct fanotify_fh is 4 bytes aligned could end up with less
>> bytes reserved for the inline buffer if the union is not also 4 bytes aligned.
>>
>> So maybe something like this:
>>
>> #define FANOTIFY_INLINE_FH(name, size) \
>>      DEFINE_FLEX_MEMBER(struct fanotify_fh, name, size) __aligned(4)
> 
> I guess you need to provide the "member" information to
> DEFINE_FLEX_MEMBER() somewhere as well.

Yeah, I can write something like that as well.

I'll come back with v2, shortly.

Thanks for the feedback, folks! :)
--
Gustavo

Powered by blists - more mailing lists