linux-kernel - Re: [PATCH][next] fanotify: Avoid a couple 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: <CAOQ4uxj-tsr5XWXfu3BHRygubA5kzZVsb_x6ELb_U_N77AA96A@mail.gmail.com>
Date: Wed, 7 May 2025 07:56:21 +0200
From: Amir Goldstein <amir73il@...il.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Jan Kara <jack@...e.cz>, 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 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).

>         } __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).

> +       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)

Thanks,
Amir.