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

PATCH In review Gustavo A. R. Silva 4 participants 7 messages Feb 6, 2025 – Feb 11, 2025 TTY layer and serial drivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2025-02-06 03:39 UTC #0 lore raw

PATCH v3 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.

Currently, member sentinel in struct tty_bufhead is causing trouble becase its type is struct tty_buffer, which is a flexible structure --meaning it contains a flexible-array member. This combined with the fact that sentinel is positioned in the middle of struct tty_bufhead, triggers hundreds of -Wflex-array-member-not-at-end warnings because flex-array members in the middle of structures are deprecated, and all code involving them should be fixed/refactored/adjusted --flex-array members are only allowed at the very end of any number of nested structures. Enabling -Wflex-array-member-not-at-end globally will enforce this behavior.

So, in this particular case, we create a new struct tty_buffer_hdr to enclose the header part of flexible structure struct tty_buffer This is all the members except the flexible array data[]. We then replace that header part with struct tty_buffer_hdr hdr; in struct tty_buffer, and change the type of sentinel from struct tty_buffer to struct tty_buffer_hdr --this bit gets rid of the flex-array-in-the-middle part.

The next step is to refactor the rest of the related code, accordingly. Which means, adding a bunch of hdr. wherever it's needed.

Lastly, we use container_of() whenever we need to retrieve a pointer to the flexible structure struct tty_buffer.

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]>

---
Changes in v3:
- Implement struct tty_buffer_hdr as a separate struct and embed it into struct tty_buffer. Refactor the rest of the code, accordingly.

Changes in v2:
- Fix a space at the beginning of the line issue, and adjust the identation of a code coment.
- Link: https://lore.kernel.org/linux-hardening/Z6L29DXeGWl-6OnK@kspp/

v1:
- Link: https://lore.kernel.org/linux-hardening/Z6L1XwE-WEzcGFwv@kspp/

 drivers/tty/tty_buffer.c   | 114 +++++++++++++++++++------------------
 include/linux/tty_buffer.h |  12 ++--
 include/linux/tty_flip.h   |  10 ++--
 3 files changed, 73 insertions(+), 63 deletions(-)

drivers/tty/tty_buffer.c +60 ~~-54~~

index 79f0ff94ce00..cd04a6567a33 100644
@@ -70,7 +70,7 @@ EXPORT_SYMBOL_GPL(tty_buffer_lock_exclusive);
 void tty_buffer_unlock_exclusive(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
-	bool restart = buf->head->commit != buf->head->read;
+	bool restart = buf->head->hdr.commit != buf->head->hdr.read;
 
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);

@@ -101,13 +101,13 @@ EXPORT_SYMBOL_GPL(tty_buffer_space_avail);
 
 static void tty_buffer_reset(struct tty_buffer *p, size_t size)
 {
-	p->used = 0;
-	p->size = size;
-	p->next = NULL;
-	p->commit = 0;
-	p->lookahead = 0;
-	p->read = 0;
-	p->flags = true;
+	p->hdr.used = 0;
+	p->hdr.size = size;
+	p->hdr.next = NULL;
+	p->hdr.commit = 0;
+	p->hdr.lookahead = 0;
+	p->hdr.read = 0;
+	p->hdr.flags = true;
 }
 
 /**

@@ -120,24 +120,27 @@ 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;
-		if (p->size > 0)
+		buf->head = p->hdr.next;
+		freed += p->hdr.size;
+		if (p->hdr.size > 0)
 			kfree(p);
 	}
 	llist = llist_del_all(&buf->free);
-	llist_for_each_entry_safe(p, next, llist, free)
+	llist_for_each_entry_safe(p, next, llist, hdr.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!",

@@ -167,7 +170,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
 	if (size <= MIN_TTYB_SIZE) {
 		free = llist_del_first(&port->buf.free);
 		if (free) {
-			p = llist_entry(free, struct tty_buffer, free);
+			p = llist_entry(free, struct tty_buffer, hdr.free);
 			goto found;
 		}
 	}

@@ -200,12 +203,12 @@ static void tty_buffer_free(struct tty_port *port, struct tty_buffer *b)
 	struct tty_bufhead *buf = &port->buf;
 
 	/* Dumb strategy for now - should keep some stats */
-	WARN_ON(atomic_sub_return(b->size, &buf->mem_used) < 0);
+	WARN_ON(atomic_sub_return(b->hdr.size, &buf->mem_used) < 0);
 
-	if (b->size > MIN_TTYB_SIZE)
+	if (b->hdr.size > MIN_TTYB_SIZE)
 		kfree(b);
-	else if (b->size > 0)
-		llist_add(&b->free, &buf->free);
+	else if (b->hdr.size > 0)
+		llist_add(&b->hdr.free, &buf->free);
 }
 
 /**

@@ -230,12 +233,12 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 	/* paired w/ release in __tty_buffer_request_room; ensures there are
 	 * no pending memory accesses to the freed buffer
 	 */
-	while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
+	while ((next = smp_load_acquire(&buf->head->hdr.next)) != NULL) {
 		tty_buffer_free(port, buf->head);
 		buf->head = next;
 	}
-	buf->head->read = buf->head->commit;
-	buf->head->lookahead = buf->head->read;
+	buf->head->hdr.read = buf->head->hdr.commit;
+	buf->head->hdr.lookahead = buf->head->hdr.read;
 
 	if (ld && ld->ops->flush_buffer)
 		ld->ops->flush_buffer(tty);

@@ -263,8 +266,8 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 {
 	struct tty_bufhead *buf = &port->buf;
 	struct tty_buffer *n, *b = buf->tail;
-	size_t left = (b->flags ? 1 : 2) * b->size - b->used;
-	bool change = !b->flags && flags;
+	size_t left = (b->hdr.flags ? 1 : 2) * b->hdr.size - b->hdr.used;
+	bool change = !b->hdr.flags && flags;
 
 	if (!change && left >= size)
 		return size;

@@ -274,19 +277,19 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 	if (n == NULL)
 		return change ? 0 : left;
 
-	n->flags = flags;
+	n->hdr.flags = flags;
 	buf->tail = n;
 	/*
 	 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
 	 * ensures they see all buffer data.
 	 */
-	smp_store_release(&b->commit, b->used);
+	smp_store_release(&b->hdr.commit, b->hdr.used);
 	/*
 	 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
 	 * ensures the latest commit value can be read before the head
 	 * is advanced to the next buffer.
 	 */
-	smp_store_release(&b->next, n);
+	smp_store_release(&b->hdr.next, n);
 
 	return size;
 }

@@ -312,19 +315,19 @@ size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
 		if (unlikely(space == 0))
 			break;
 
-		memcpy(char_buf_ptr(tb, tb->used), chars, space);
+		memcpy(char_buf_ptr(tb, tb->hdr.used), chars, space);
 
 		if (mutable_flags) {
-			memcpy(flag_buf_ptr(tb, tb->used), flags, space);
+			memcpy(flag_buf_ptr(tb, tb->hdr.used), flags, space);
 			flags += space;
-		} else if (tb->flags) {
-			memset(flag_buf_ptr(tb, tb->used), flags[0], space);
+		} else if (tb->hdr.flags) {
+			memset(flag_buf_ptr(tb, tb->hdr.used), flags[0], space);
 		} else {
 			/* tb->flags should be available once requested */
 			WARN_ON_ONCE(need_flags);
 		}
 
-		tb->used += space;
+		tb->hdr.used += space;
 		copied += space;
 		chars += space;

@@ -358,10 +361,10 @@ size_t tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size)
 	if (likely(space)) {
 		struct tty_buffer *tb = port->buf.tail;
 
-		*chars = char_buf_ptr(tb, tb->used);
-		if (tb->flags)
-			memset(flag_buf_ptr(tb, tb->used), TTY_NORMAL, space);
-		tb->used += space;
+		*chars = char_buf_ptr(tb, tb->hdr.used);
+		if (tb->hdr.flags)
+			memset(flag_buf_ptr(tb, tb->hdr.used), TTY_NORMAL, space);
+		tb->hdr.used += space;
 	}
 
 	return space;

@@ -396,7 +399,7 @@ EXPORT_SYMBOL_GPL(tty_ldisc_receive_buf);
 
 static void lookahead_bufs(struct tty_port *port, struct tty_buffer *head)
 {
-	head->lookahead = max(head->lookahead, head->read);
+	head->hdr.lookahead = max(head->hdr.lookahead, head->hdr.read);
 
 	while (head) {
 		struct tty_buffer *next;

@@ -407,12 +410,12 @@ static void lookahead_bufs(struct tty_port *port, struct tty_buffer *head)
 		 * ensures commit value read is not stale if the head
 		 * is advancing to the next buffer.
 		 */
-		next = smp_load_acquire(&head->next);
+		next = smp_load_acquire(&head->hdr.next);
 		/*
 		 * Paired w/ release in __tty_buffer_request_room() or in
 		 * tty_buffer_flush(); ensures we see the committed buffer data.
 		 */
-		count = smp_load_acquire(&head->commit) - head->lookahead;
+		count = smp_load_acquire(&head->hdr.commit) - head->hdr.lookahead;
 		if (!count) {
 			head = next;
 			continue;

@@ -421,26 +424,26 @@ static void lookahead_bufs(struct tty_port *port, struct tty_buffer *head)
 		if (port->client_ops->lookahead_buf) {
 			u8 *p, *f = NULL;
 
-			p = char_buf_ptr(head, head->lookahead);
-			if (head->flags)
-				f = flag_buf_ptr(head, head->lookahead);
+			p = char_buf_ptr(head, head->hdr.lookahead);
+			if (head->hdr.flags)
+				f = flag_buf_ptr(head, head->hdr.lookahead);
 
 			port->client_ops->lookahead_buf(port, p, f, count);
 		}
 
-		head->lookahead += count;
+		head->hdr.lookahead += count;
 	}
 }
 
 static size_t
 receive_buf(struct tty_port *port, struct tty_buffer *head, size_t count)
 {
-	u8 *p = char_buf_ptr(head, head->read);
+	u8 *p = char_buf_ptr(head, head->hdr.read);
 	const u8 *f = NULL;
 	size_t n;
 
-	if (head->flags)
-		f = flag_buf_ptr(head, head->read);
+	if (head->hdr.flags)
+		f = flag_buf_ptr(head, head->hdr.read);
 
 	n = port->client_ops->receive_buf(port, p, f, count);
 	if (n > 0)

@@ -479,11 +482,11 @@ static void flush_to_ldisc(struct work_struct *work)
 		 * ensures commit value read is not stale if the head
 		 * is advancing to the next buffer
 		 */
-		next = smp_load_acquire(&head->next);
+		next = smp_load_acquire(&head->hdr.next);
 		/* paired w/ release in __tty_buffer_request_room() or in
 		 * tty_buffer_flush(); ensures we see the committed buffer data
 		 */
-		count = smp_load_acquire(&head->commit) - head->read;
+		count = smp_load_acquire(&head->hdr.commit) - head->hdr.read;
 		if (!count) {
 			if (next == NULL)
 				break;

@@ -493,7 +496,7 @@ static void flush_to_ldisc(struct work_struct *work)
 		}
 
 		rcvd = receive_buf(port, head, count);
-		head->read += rcvd;
+		head->hdr.read += rcvd;
 		if (rcvd < count)
 			lookahead_bufs(port, head);
 		if (!rcvd)

@@ -513,7 +516,7 @@ static inline void tty_flip_buffer_commit(struct tty_buffer *tail)
 	 * Paired w/ acquire in flush_to_ldisc(); ensures flush_to_ldisc() sees
 	 * buffer data.
 	 */
-	smp_store_release(&tail->commit, tail->used);
+	smp_store_release(&tail->hdr.commit, tail->hdr.used);
 }
 
 /**

@@ -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 +8 -4

index 31125e3be3c5..80a9d7832c97 100644
@@ -7,7 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 
-struct tty_buffer {
+struct tty_buffer_hdr {
 	union {
 		struct tty_buffer *next;
 		struct llist_node free;

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

@@ -29,7 +33,7 @@ static inline u8 *char_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 
 static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 {
-	return char_buf_ptr(b, ofs) + b->size;
+	return char_buf_ptr(b, ofs) + b->hdr.size;
 }
 
 struct tty_bufhead {

@@ -37,7 +41,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;

include/linux/tty_flip.h +5 -5

index af4fce98f64e..1874b0059e97 100644
@@ -67,11 +67,11 @@ static inline size_t tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 	struct tty_buffer *tb = port->buf.tail;
 	int change;
 
-	change = !tb->flags && (flag != TTY_NORMAL);
-	if (!change && tb->used < tb->size) {
-		if (tb->flags)
-			*flag_buf_ptr(tb, tb->used) = flag;
-		*char_buf_ptr(tb, tb->used++) = ch;
+	change = !tb->hdr.flags && (flag != TTY_NORMAL);
+	if (!change && tb->hdr.used < tb->hdr.size) {
+		if (tb->hdr.flags)
+			*flag_buf_ptr(tb, tb->hdr.used) = flag;
+		*char_buf_ptr(tb, tb->hdr.used++) = ch;
 		return 1;
 	}
 	return __tty_insert_flip_string_flags(port, &ch, &flag, false, 1);

Signature · 1 line

2.43.0

Jiri Slaby

maintainer

2025-02-06 09:12 UTC #1 lore raw

Quoted 4 lines · Gustavo A. R. Silva

Currently, member sentinel in struct tty_bufhead is causing trouble becase its type is struct tty_buffer, which is a flexible structure --meaning it contains a flexible-array member. This combined with the fact that sentinel is positioned in the middle of struct tty_bufhead,

THUMBS_UP

Quoted 6 lines

Suggested-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v3:
- Implement struct tty_buffer_hdr as a separate struct and embed it into struct tty_buffer. Refactor the rest of the code, accordingly.

Better, but:

Quoted 4 lines

drivers/tty/tty_buffer.c +0 -0
index 79f0ff94ce00..cd04a6567a33 100644

...

drivers/tty/tty_buffer.c int tty_insert_flip_string_and_push_buffer(struct tty_port *port, · line 584

Quoted 7 lines

drivers/tty/tty_buffer.c +3 -0

@@ -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);

Bah, so this is ugly and even dangerous if someone adds a member to tty_buffer outside _hdr.

We should link headers in the list, it appears.

Quoted 12 lines

include/linux/tty_buffer.h +1 -1

@@ -7,7 +7,7 @@
  #include <linux/mutex.h>
  #include <linux/workqueue.h>
 
-struct tty_buffer {
+struct tty_buffer_hdr {
  	union {
  		struct tty_buffer *next;
  		struct llist_node free;

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

Quoted 5 lines

include/linux/tty_buffer.h +1 -1

@@ -15,9 +15,13 @@ struct tty_buffer {
  	unsigned int used;
  	unsigned int size;
  	unsigned int commit;
-	unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
+	unsigned int lookahead; /* Lazy update on recv, can become less than "read" */

This is an unrelated/unwanted change.

thanks,

Signature · 2 lines

js
suse labs

Gustavo A. R. Silva

2025-02-06 09:23 UTC #2 lore raw

Quoted 11 lines

drivers/tty/tty_buffer.c int tty_insert_flip_string_and_push_buffer(struct tty_port *port, · line 584
Quoted 9 lines
drivers/tty/tty_buffer.c +3 -0
@@ -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);
Bah, so this is ugly and even dangerous if someone adds a member to tty_buffer outside _hdr.

Precisely. :) This is one (more) of the reasons why of the struct_group()/static_assert() proposed changes.

-Gustavo

Gustavo A. R. Silva

2025-02-11 07:03 UTC #3 lore raw

In reply to Jiri Slaby

Quoted 14 lines

drivers/tty/tty_buffer.c int tty_insert_flip_string_and_push_buffer(struct tty_port *port, · line 584
Quoted 11 lines
drivers/tty/tty_buffer.c +3 -0
@@ -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);
Bah, so this is ugly and even dangerous if someone adds a member to tty_buffer outside _hdr.

Quoted 1 line

We should link headers in the list, it appears.

What should that patch look like?

--
Gustavo

Greg Kroah-Hartman

maintainer

2025-02-06 14:27 UTC #4 lore raw

In reply to Gustavo A. R. Silva

Quoted 14 lines · Gustavo A. R. Silva

include/linux/tty_buffer.h +1 -1

index 31125e3be3c5..80a9d7832c97 100644
@@ -7,7 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 
-struct tty_buffer {
+struct tty_buffer_hdr {
 	union {
 		struct tty_buffer *next;
 		struct llist_node free;

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

Quoted 5 lines

include/linux/tty_buffer.h +1 -1

@@ -15,9 +15,13 @@ struct tty_buffer {
 	unsigned int used;
 	unsigned int size;
 	unsigned int commit;
-	unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
+	unsigned int lookahead; /* Lazy update on recv, can become less than "read" */

Coding style change?

Gustavo A. R. Silva

2025-02-06 23:23 UTC #5 lore raw

Quoted 8 lines

include/linux/tty_buffer.h struct tty_buffer { · line 18
Quoted 6 lines
unsigned int used;
unsigned int size;
unsigned int commit;
include/linux/tty_buffer.h +1 -1
-	unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
+	unsigned int lookahead; /* Lazy update on recv, can become less than "read" */
Coding style change?

(Whaa? for some reason your response didn't reach my inbox until today)

It seems I got a bit too proactive.

What do you think about the rest of the patch?

Thanks!
-Gustavo

Greg Kroah-Hartman

maintainer

2025-02-07 09:38 UTC #6 lore raw

Quoted 10 lines · Gustavo A. R. Silva

Quoted 8 lines
include/linux/tty_buffer.h struct tty_buffer { · line 18
Quoted 6 lines
unsigned int used;
unsigned int size;
unsigned int commit;
include/linux/tty_buffer.h +1 -1
-	unsigned int lookahead;		/* Lazy update on recv, can become less than "read" */
+	unsigned int lookahead; /* Lazy update on recv, can become less than "read" */
Coding style change?
(Whaa? for some reason your response didn't reach my inbox until today)

Sorry, my email was stuck on my system for a day, my fault.

Quoted 3 lines

It seems I got a bit too proactive.
What do you think about the rest of the patch?

I agree with Jiri's comments, and overall I like it as a way forward.

thanks,

greg k-h