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

[nathanchance]

After https://github.com/llvm/llvm-project/commit/cef6387e52578366c2332275dad88b9953b55336, 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/