[PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[Gustavo A. R. Silva]

-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.

There is currently a local structure `f` that is using a flexible
`struct file_handle` as header for an on-stack place-holder for the
flexible-array member `unsigned char f_handle[];`.

struct {
	struct file_handle handle;
	u8 pad[MAX_HANDLE_SZ];
} f;

However, we are deprecating flexible arrays in the middle of another
struct. So, in order to avoid this, we use the `struct_group_tagged()`
helper to separate the flexible array from the rest of the members in
the flexible structure:

struct file_handle {
        struct_group_tagged(file_handle_hdr, hdr,
		... the rest of the members
        );
        unsigned char f_handle[];
};

With the change described above, we can now declare an object of the
type of the tagged struct, without embedding the flexible array in the
middle of another struct:

struct {
        struct file_handle_hdr handle;
        u8 pad[MAX_HANDLE_SZ];
} f;

We also use `container_of()` whenever we need to retrieve a pointer to
the flexible structure, through which the flexible-array member can be
accessed, as in this case.

So, with these changes, fix the following warning:

fs/notify/fdinfo.c: In function ‘show_mark_fhandle’:
fs/notify/fdinfo.c:45:36: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
   45 |                 struct file_handle handle;
      |                                    ^~~~~~

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 fs/notify/fdinfo.c | 8 +++++---
 include/linux/fs.h | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 5c430736ec12..740f5e68b397 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -42,15 +42,17 @@ static void show_fdinfo(struct seq_file *m, struct file *f,
 static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 {
 	struct {
-		struct file_handle handle;
+		struct file_handle_hdr handle;
 		u8 pad[MAX_HANDLE_SZ];
 	} f;
+	struct file_handle *handle = container_of(&f.handle,
+						  struct file_handle, hdr);
 	int size, ret, i;
 
 	f.handle.handle_bytes = sizeof(f.pad);
 	size = f.handle.handle_bytes >> 2;
 
-	ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
+	ret = exportfs_encode_fid(inode, (struct fid *)handle->f_handle, &size);
 	if ((ret == FILEID_INVALID) || (ret < 0)) {
 		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
 		return;
@@ -63,7 +65,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 		   f.handle.handle_bytes, f.handle.handle_type);
 
 	for (i = 0; i < f.handle.handle_bytes; i++)
-		seq_printf(m, "%02x", (int)f.handle.f_handle[i]);
+		seq_printf(m, "%02x", (int)handle->f_handle[i]);
 }
 #else
 static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 00fc429b0af0..7c131bcd948f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1030,8 +1030,10 @@ struct file {
   __attribute__((aligned(4)));	/* lest something weird decides that 2 is OK */
 
 struct file_handle {
-	__u32 handle_bytes;
-	int handle_type;
+	struct_group_tagged(file_handle_hdr, hdr,
+		__u32 handle_bytes;
+		int handle_type;
+	);
 	/* file identifier */
 	unsigned char f_handle[];
 };
-- 
2.34.1

Re: [PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[Kees Cook]

On Tue, Mar 05, 2024 at 04:18:46PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
> 
> There is currently a local structure `f` that is using a flexible
> `struct file_handle` as header for an on-stack place-holder for the
> flexible-array member `unsigned char f_handle[];`.
> 
> struct {
> 	struct file_handle handle;
> 	u8 pad[MAX_HANDLE_SZ];
> } f;

This code pattern is "put a flex array struct on the stack", but we have
a macro for this now:

DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ);

And you can even include the initializer:

_DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ,
	     = { .handle_bytes = MAX_HANDLE_SZ });

I think this would be a simpler conversion.

Also, this could use a __counted_by tag...

I need to improve the DEFINE_FLEX macro a bit, though, to take advantage
of __counted_by.

-- 
Kees Cook

Re: [PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[Amir Goldstein]

On Wed, Mar 6, 2024 at 1:52 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Mar 05, 2024 at 04:18:46PM -0600, Gustavo A. R. Silva wrote:
> > -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> > ready to enable it globally.
> >
> > There is currently a local structure `f` that is using a flexible
> > `struct file_handle` as header for an on-stack place-holder for the
> > flexible-array member `unsigned char f_handle[];`.
> >
> > struct {
> >       struct file_handle handle;
> >       u8 pad[MAX_HANDLE_SZ];
> > } f;
>
> This code pattern is "put a flex array struct on the stack", but we have
> a macro for this now:
>
> DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ);
>
> And you can even include the initializer:
>
> _DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ,
>              = { .handle_bytes = MAX_HANDLE_SZ });
>

Indeed that looks much nicer.

Thanks,
Amir.

Re: [PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[Gustavo A. R. Silva]

On 3/6/24 01:36, Amir Goldstein wrote:
> On Wed, Mar 6, 2024 at 1:52 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Tue, Mar 05, 2024 at 04:18:46PM -0600, Gustavo A. R. Silva wrote:
>>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
>>> ready to enable it globally.
>>>
>>> There is currently a local structure `f` that is using a flexible
>>> `struct file_handle` as header for an on-stack place-holder for the
>>> flexible-array member `unsigned char f_handle[];`.
>>>
>>> struct {
>>>        struct file_handle handle;
>>>        u8 pad[MAX_HANDLE_SZ];
>>> } f;
>>
>> This code pattern is "put a flex array struct on the stack", but we have
>> a macro for this now:
>>
>> DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ);
>>
>> And you can even include the initializer:
>>
>> _DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ,
>>               = { .handle_bytes = MAX_HANDLE_SZ });
>>
> 
> Indeed that looks much nicer.


Yeah, I'll probably wait for this to land before I send a v2:

https://lore.kernel.org/linux-hardening/20240306010746.work.678-kees@kernel.org/

Thanks
--
Gustavo

Re: [PATCH][next] fsnotify: Avoid -Wflex-array-member-not-at-end warning

[Gustavo A. R. Silva]

On 3/5/24 17:52, Kees Cook wrote:
> On Tue, Mar 05, 2024 at 04:18:46PM -0600, Gustavo A. R. Silva wrote:
>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
>> ready to enable it globally.
>>
>> There is currently a local structure `f` that is using a flexible
>> `struct file_handle` as header for an on-stack place-holder for the
>> flexible-array member `unsigned char f_handle[];`.
>>
>> struct {
>> 	struct file_handle handle;
>> 	u8 pad[MAX_HANDLE_SZ];
>> } f;
> 
> This code pattern is "put a flex array struct on the stack", but we have
> a macro for this now:
> 
> DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ);
> 
> And you can even include the initializer:
> 
> _DEFINE_FLEX(struct file_handle, handle, f_handle, MAX_HANDLE_SZ,
> 	     = { .handle_bytes = MAX_HANDLE_SZ });
> 
> I think this would be a simpler conversion.
> 
> Also, this could use a __counted_by tag...
> 
> I need to improve the DEFINE_FLEX macro a bit, though, to take advantage
> of __counted_by.
> 

Yep, I like it.

I'll go and hunt down all those on-stack -Wflex-array-member-not-at-end
issues with this helper. :)

Thanks
--
Gustavo