| Message ID | ZeeaRuTpuxInH6ZB@neat (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [next] fsnotify: Avoid -Wflex-array-member-not-at-end warning | expand |
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.
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.
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
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
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[]; };
-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(-)