tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings

PATCH Superseded Gustavo A. R. Silva 4 participants 9 messages Feb 5, 2025 TTY layer and serial drivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2025-02-05 05:21 UTC #0 lore raw

PATCH 1/1 tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings

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

So, in order to avoid ending up with a flexible-array member in the middle of other structs, we use the struct_group_tagged() helper to create a new tagged struct tty_buffer_hdr. This structure groups together all the members of the flexible struct tty_buffer except the flexible array.

As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct member currently causing trouble from struct tty_buffer to struct tty_buffer_hdr.

We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use static_assert(). This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes.

This approach avoids having to implement struct tty_buffer_hdr as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future.

We also use container_of() whenever we need to retrieve a pointer to the flexible structure, through which we can access the flexible-array member, if necessary.

So, with these changes, fix 384 of the following warnings:
include/linux/tty_buffer.h:40:27: 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 <[email protected]>

---
 drivers/tty/tty_buffer.c   | 18 ++++++++++------
 include/linux/tty_buffer.h | 43 +++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 25 deletions(-)

drivers/tty/tty_buffer.c +12 -6

index 79f0ff94ce00..f4e7520df7b1 100644
@@ -120,11 +120,14 @@ static void tty_buffer_reset(struct tty_buffer *p, size_t size)
 void tty_buffer_free_all(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
+	struct tty_buffer *buf_sentinel;
 	struct tty_buffer *p, *next;
 	struct llist_node *llist;
 	unsigned int freed = 0;
 	int still_used;
 
+	buf_sentinel = container_of(&buf->sentinel, struct tty_buffer, __hdr);
+
 	while ((p = buf->head) != NULL) {
 		buf->head = p->next;
 		freed += p->size;

@@ -135,9 +138,9 @@ void tty_buffer_free_all(struct tty_port *port)
 	llist_for_each_entry_safe(p, next, llist, free)
 		kfree(p);
 
-	tty_buffer_reset(&buf->sentinel, 0);
-	buf->head = &buf->sentinel;
-	buf->tail = &buf->sentinel;
+	tty_buffer_reset(buf_sentinel, 0);
+	buf->head = buf_sentinel;
+	buf->tail = buf_sentinel;
 
 	still_used = atomic_xchg(&buf->mem_used, 0);
 	WARN(still_used != freed, "we still have not freed %d bytes!",

@@ -576,11 +579,14 @@ int tty_insert_flip_string_and_push_buffer(struct tty_port *port,
 void tty_buffer_init(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
+	struct tty_buffer *buf_sentinel;
+
+	buf_sentinel = container_of(&buf->sentinel, struct tty_buffer, __hdr);
 
 	mutex_init(&buf->lock);
-	tty_buffer_reset(&buf->sentinel, 0);
-	buf->head = &buf->sentinel;
-	buf->tail = &buf->sentinel;
+	tty_buffer_reset(buf_sentinel, 0);
+	buf->head = buf_sentinel;
+	buf->tail = buf_sentinel;
 	init_llist_head(&buf->free);
 	atomic_set(&buf->mem_used, 0);
 	atomic_set(&buf->priority, 0);

include/linux/tty_buffer.h +24 ~~-19~~

58 lines not on this page · raw message

Signature · 1 line

2.43.0

Greg Kroah-Hartman

maintainer

2025-02-05 05:33 UTC #1 lore raw

Quoted 7 lines · Gustavo A. R. Silva

include/linux/tty_buffer.h +0 -1

@@ -8,19 +8,24 @@
 #include <linux/workqueue.h>
 
 struct tty_buffer {
-	union {

include/linux/tty_buffer.h line 11

Quoted 10 lines

include/linux/tty_buffer.h +1 -9

-		struct tty_buffer *next;
-		struct llist_node free;
-	};
-	unsigned int used;
-	unsigned int size;
-	unsigned int commit;
-	unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
-	unsigned int read;
-	bool flags;
+	/* New members MUST be added within the struct_group() macro below. */

Why? You need to say that here otherwise we aren't going to know.

Quoted 33 lines

include/linux/tty_buffer.h +14 -9

+	struct_group_tagged(tty_buffer_hdr, __hdr,
+		union {
+			struct tty_buffer *next;
+			struct llist_node free;
+		};
+		unsigned int used;
+		unsigned int size;
+		unsigned int commit;
+		unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
+		unsigned int read;
+		bool flags;
+	);
 	/* Data points here */
 	u8 data[] __aligned(sizeof(unsigned long));
 };
+static_assert(offsetof(struct tty_buffer, data) == sizeof(struct tty_buffer_hdr),
+              "struct member likely outside of struct_group_tagged()");
 
 static inline u8 *char_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 {

@@ -33,15 +38,15 @@ static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 }
 
 struct tty_bufhead {
-	struct tty_buffer *head;	/* Queue head */
-	struct work_struct work;
-	struct mutex	   lock;
-	atomic_t	   priority;
-	struct tty_buffer sentinel;
-	struct llist_head free;		/* Free queue head */
-	atomic_t	   mem_used;    /* In-use buffers excluding free list */
-	int		   mem_limit;
-	struct tty_buffer *tail;	/* Active buffer */

include/linux/tty_buffer.h static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs) · line 50

Quoted 10 lines

include/linux/tty_buffer.h +9 -0

+	struct tty_buffer	*head;	/* Queue head */
+	struct work_struct	work;
+	struct mutex		lock;
+	atomic_t		priority;
+	struct tty_buffer_hdr	sentinel;
+	struct llist_head	free;		/* Free queue head */
+	atomic_t		mem_used;    /* In-use buffers excluding free list */
+	int			mem_limit;
+	struct tty_buffer	*tail;	/* Active buffer */
 };

Did you change anything in this structure? By reformatting it, it's hard to tell what happened, so please don't do that :(

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2025-02-05 05:36 UTC #2 lore raw

In reply to Gustavo A. R. Silva

Quoted 20 lines · Gustavo A. R. Silva

-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally.
So, in order to avoid ending up with a flexible-array member in the middle of other structs, we use the struct_group_tagged() helper to create a new tagged struct tty_buffer_hdr. This structure groups together all the members of the flexible struct tty_buffer except the flexible array.
As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct member currently causing trouble from struct tty_buffer to struct tty_buffer_hdr.
We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use static_assert(). This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes.

Quoted 4 lines

This approach avoids having to implement struct tty_buffer_hdr as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future.

Why not just have a separate structure and embed that in the places it is used? No duplication should be needed or am I missing something?

I don't mind that, it would make this all much simpler and more obvious over time, and the tty layer needs all the "simplification" it can get :)

thanks,

greg k-h

Jiri Slaby

maintainer

2025-02-05 06:45 UTC #3 lore raw

Quoted 26 lines · Greg Kroah-Hartman

Quoted 20 lines · Gustavo A. R. Silva
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally.
So, in order to avoid ending up with a flexible-array member in the middle of other structs, we use the struct_group_tagged() helper to create a new tagged struct tty_buffer_hdr. This structure groups together all the members of the flexible struct tty_buffer except the flexible array.
As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct member currently causing trouble from struct tty_buffer to struct tty_buffer_hdr.
We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use static_assert(). This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes.
Quoted 4 lines
This approach avoids having to implement struct tty_buffer_hdr as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future.

Quoted 6 lines

Why not just have a separate structure and embed that in the places it is used? No duplication should be needed or am I missing something?
I don't mind that, it would make this all much simpler and more obvious over time, and the tty layer needs all the "simplification" it can get :)

+100. You can name the member hdr or even h. Another approach would be to get rid of sentinel completely. But that might be too hard. Have you looked into it? You should describe that above too.

On the top of that: I remember I already looked into this when gcc14 was introduced and I was retracted by something else. Nevertheless, it took me quite a while to understand what the exact problem is and how you are doing the fix.

Both from the patch (the main change in tty_bufhead is hidden behind whitespace changes) and especially from the description (you do not say the simple: tty_bufhead contains data[] in the middle due to embedded tty_buffer there). Both need to be improved.

PS v2 was sent too early :P.

thanks,

Signature · 2 lines

js
suse labs

Gustavo A. R. Silva

2025-02-05 06:49 UTC #4 lore raw

In reply to Greg Kroah-Hartman

Quoted 21 lines · Greg Kroah-Hartman

Quoted 20 lines · Gustavo A. R. Silva
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally.
So, in order to avoid ending up with a flexible-array member in the middle of other structs, we use the struct_group_tagged() helper to create a new tagged struct tty_buffer_hdr. This structure groups together all the members of the flexible struct tty_buffer except the flexible array.
As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct member currently causing trouble from struct tty_buffer to struct tty_buffer_hdr.
We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use static_assert(). This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes.

Quoted 7 lines

Quoted 4 lines
This approach avoids having to implement struct tty_buffer_hdr as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future.
Why not just have a separate structure and embed that in the places it is used? No duplication should be needed or am I missing something?

The duplication of members in struct tty_buffer (all of them except the flexible array) and the new separate struct looks like this:

include/linux/tty_buffer.h +14 -1

index 31125e3be3c5..6f47a6ea5a05 100644
@@ -22,6 +22,19 @@ struct tty_buffer {
        u8 data[] __aligned(sizeof(unsigned long));
 };
 
+struct tty_buffer_hdr {
+        union {
+                struct tty_buffer *next;
+                struct llist_node free;
+        };
+        unsigned int used;
+        unsigned int size;
+        unsigned int commit;
+        unsigned int lookahead;         /* Lazy update on recv, can become less than "read" */
+        unsigned int read;
+        bool flags;
+};
+
 static inline u8 *char_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 {
        return b->data + ofs;

@@ -37,7 +50,7 @@ struct tty_bufhead {
        struct work_struct work;
        struct mutex       lock;
        atomic_t           priority;
-       struct tty_buffer sentinel;
+       struct tty_buffer_hdr sentinel;
        struct llist_head free;         /* Free queue head */
        atomic_t           mem_used;    /* In-use buffers excluding free list */
        int                mem_limit;

Quoted 2 lines

Did you change anything in this structure? By reformatting it, it's hard to tell what happened, so please don't do that :(

Yes - the above change is the reason why I reformatted the whole thing.

Quoted 4 lines

I don't mind that, it would make this all much simpler and more obvious over time, and the tty layer needs all the "simplification" it can get :)

If the above changes are better for you then I'll send a new patch. :)

Thanks
-Gustavo

Jiri Slaby

maintainer

2025-02-05 06:59 UTC #5 lore raw

Quoted 1 line · Gustavo A. R. Silva

If the above changes are better for you then I'll send a new patch. :)

No, you are supposed to switch tty_buffer to tty_buffer_hdr too.

thanks,

Signature · 2 lines

js
suse labs

Gustavo A. R. Silva

2025-02-05 08:03 UTC #6 lore raw

Quoted 4 lines · Jiri Slaby

Quoted 1 line · Gustavo A. R. Silva
If the above changes are better for you then I'll send a new patch. :)
No, you are supposed to switch tty_buffer to tty_buffer_hdr too.

Do you mean something like the following:

struct tty_buffer {
- union {
- struct tty_buffer *next;
- struct llist_node free;

-       };
-       unsigned int used;
-       unsigned int size;
-       unsigned int commit;
-       unsigned int lookahead;         /* Lazy update on recv, can become less than "read" */
-       unsigned int read;
-       bool flags;
+       struct tty_buffer_hdr hdr;
        /* Data points here */
        u8 data[] __aligned(sizeof(unsigned long));

};

+struct tty_buffer_hdr {
+ union {
+ struct tty_buffer *next;
+ struct llist_node free;

+        };
+        unsigned int used;
+        unsigned int size;
+        unsigned int commit;
+        unsigned int lookahead; /* Lazy update on recv, can become less than "read" */
+        unsigned int read;
+        bool flags;

+};
+

The problem with this is that then we have to modify a lot of lines from, let's say, instance->used, instance->size, etc... to instance->hdr.used, instance->hdr.size, and so on...

This code churn is avoided if we use the struct_group() helper.

However, I'm okay with whatever you guys prefer, just let me know.

Thanks
-Gustavo

Greg Kroah-Hartman

maintainer

2025-02-05 08:16 UTC #7 lore raw

Quoted 29 lines · Gustavo A. R. Silva

Quoted 4 lines · Jiri Slaby

Quoted 1 line · Gustavo A. R. Silva
If the above changes are better for you then I'll send a new patch. :)
No, you are supposed to switch tty_buffer to tty_buffer_hdr too.

Do you mean something like the following:

struct tty_buffer {

include/linux/tty_buffer.h +4 ~~-10~~

-       union {
-               struct tty_buffer *next;
-               struct llist_node free;
-       };
-       unsigned int used;
-       unsigned int size;
-       unsigned int commit;
-       unsigned int lookahead;         /* Lazy update on recv, can become less than "read" */
-       unsigned int read;
-       bool flags;
+       struct tty_buffer_hdr hdr;
        /* Data points here */
        u8 data[] __aligned(sizeof(unsigned long));
 };
 
+struct tty_buffer_hdr {
+        union {
+                struct tty_buffer *next;

include/linux/tty_buffer.h struct tty_buffer { · line 37

Quoted 10 lines

include/linux/tty_buffer.h +10 -0

+                struct llist_node free;
+        };
+        unsigned int used;
+        unsigned int size;
+        unsigned int commit;
+        unsigned int lookahead; /* Lazy update on recv, can become less than "read" */
+        unsigned int read;
+        bool flags;
+};
+

Yes!

Quoted 3 lines

The problem with this is that then we have to modify a lot of lines from, let's say, instance->used, instance->size, etc... to instance->hdr.used, instance->hdr.size, and so on...

Great, let's do that and get it right please.

Quoted 1 line

This code churn is avoided if we use the struct_group() helper.

It's not "churn" if it is "fix the code to be correct" :)

thanks,

greg k-h

Gustavo A. R. Silva

2025-02-05 08:25 UTC #8 lore raw

Quoted 7 lines

Quoted 5 lines
The problem with this is that then we have to modify a lot of lines from, let's say, instance->used, instance->size, etc... to instance->hdr.used, instance->hdr.size, and so on...
Great, let's do that and get it right please.

Deal!

Quoted 4 lines

Quoted 2 lines
This code churn is avoided if we use the struct_group() helper.
It's not "churn" if it is "fix the code to be correct" :)

As long as those warnings are fixed and we harden the kernel, I'm really okay with whatever maintainers prefer. :)

Thanks!
-Gustavo