-Wbounds-safety-counted-by-elt-type-unknown-size in drivers/tty/mxser.c

[nathanchance]

After llvm/llvm-project@cef6387, I see the following warning in trees that contain https://git.kernel.org/linus/f34907ecca71177a438bc1f1012e945326f707c3:

drivers/tty/mxser.c:291:2: warning: 'counted_by' should not be applied to an array with element of unknown size because 'struct mxser_port' is a struct type with a flexible array member. This will be an error in a future compiler version [-Wbounds-safety-counted-by-elt-type-unknown-size]
  291 |         struct mxser_port ports[] __counted_by(nports);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This seems odd to me, as it does not seem like struct mxser_port has a flexible array member but maybe it is either from the pointer to struct mxser_board or some other structure?

struct mxser_port {
	struct tty_port port;
	struct mxser_board *board;

	unsigned long ioaddr;
	unsigned long opmode_ioaddr;

	u8 rx_high_water;
	u8 rx_low_water;
	int type;		/* UART type */

	u8 x_char;		/* xon/xoff character */
	u8 IER;			/* Interrupt Enable Register */
	u8 MCR;			/* Modem control register */
	u8 FCR;			/* FIFO control register */

	struct async_icount icount; /* kernel counters for 4 input interrupts */
	unsigned int timeout;

	u8 read_status_mask;
	u8 ignore_status_mask;
	u8 xmit_fifo_size;

	spinlock_t slock;
};

struct mxser_board {
	unsigned int idx;
	unsigned short nports;
	int irq;
	unsigned long vector;

	enum mxser_must_hwid must_hwid;
	speed_t max_baud;

	struct mxser_port ports[] __counted_by(nports);
};

cc @kees @bwendling

[nathanchance]

Okay, I think I see the flexible array, it comes from the struct tty_buffer sentinel member in struct tty_bufhead buf within struct tty_port? So it seems like __counted_by(nports) is no longer acceptable in that case?

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;
	/* Data points here */
	u8 data[] __aligned(sizeof(unsigned long));
};

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 */
};

struct tty_port {
	struct tty_bufhead	buf;
	struct tty_struct	*tty;
	struct tty_struct	*itty;
	const struct tty_port_operations *ops;
	const struct tty_port_client_operations *client_ops;
	spinlock_t		lock;
	int			blocked_open;
	int			count;
	wait_queue_head_t	open_wait;
	wait_queue_head_t	delta_msr_wait;
	unsigned long		flags;
	unsigned long		iflags;
	unsigned char		console:1;
	struct mutex		mutex;
	struct mutex		buf_mutex;
	u8			*xmit_buf;
	DECLARE_KFIFO_PTR(xmit_fifo, u8);
	unsigned int		close_delay;
	unsigned int		closing_wait;
	int			drain_delay;
	struct kref		kref;
	void			*client_data;
};

[bwendling]

?!?! Who wrote this atrocity? The only thing I can imagine that it's doing is treating the rest of struct tty_bufhead (free to tail) as data, which is just sad...

I'm not sure what to do in this situation. Obviously a flexible array isn't valid in the middle of a struct, so maybe the check should only check for a flexible array that happens at the end of the struct?

[bwendling]

Oh, this also means that the counted_by checks probably won't be correct for sentinel.data...

[nathanchance]

Should this be the patch then?

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 458bb1280ebf..2d6afd2b220d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -288,7 +288,7 @@ struct mxser_board {
        enum mxser_must_hwid must_hwid;
        speed_t max_baud;

-       struct mxser_port ports[] __counted_by(nports);
+       struct mxser_port ports[] /* __counted_by(nports) */;
 };

 static DECLARE_BITMAP(mxser_boards, MXSER_BOARDS);

[nathanchance]

This is now breaking CI because of -Werror: https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/9261999992

[nathanchance]

Patch sent: https://lore.kernel.org/20240529-drop-counted-by-ports-mxser-board-v1-1-0ab217f4da6d@kernel.org/

[GustavoARSilva]

I wonder if the warning disappears when the following patch is applied:

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 79f0ff94ce00..5837aab62053 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -121,6 +121,8 @@ void tty_buffer_free_all(struct tty_port *port)
 {
        struct tty_bufhead *buf = &port->buf;
        struct tty_buffer *p, *next;
+       struct tty_buffer *buf_sentinel = container_of(&buf->sentinel,
+                                               struct tty_buffer, hdr);
        struct llist_node *llist;
        unsigned int freed = 0;
        int still_used;
@@ -135,9 +137,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 +578,13 @@ 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 = 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);
diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
index 31125e3be3c5..f9c11906fe07 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -8,16 +8,19 @@
 #include <linux/workqueue.h>
 
 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;
+       /* New members must be added within the struct_group() macro below. */
+       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));
 };
@@ -37,7 +40,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;

[nathanchance]

This is now resolved in mainline: https://git.kernel.org/linus/1c07c9be87dd3dd0634033bf08728b32465f08fb

It is out for review in stable: https://lore.kernel.org/stable/20240702170250.169388429@linuxfoundation.org/