[PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Nathan Chancellor]

Work for __counted_by on generic pointers in structures (not just
flexible array members) has started landing in Clang 19 (current tip of
tree). During the development of this feature, a restriction was added
to __counted_by to prevent the flexible array member's element type from
including a flexible array member itself such as:

  struct foo {
    int count;
    char buf[];
  };

  struct bar {
    int count;
    struct foo data[] __counted_by(count);
  };

because the size of data cannot be calculated with the standard array
size formula:

  sizeof(struct foo) * count

This restriction was downgraded to a warning but due to CONFIG_WERROR,
it can still break the build. The application of __counted_by on the
ports member of 'struct mxser_board' triggers this restriction,
resulting in:

  drivers/tty/mxser.c:291:2: error: '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 [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
    291 |         struct mxser_port ports[] __counted_by(nports);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~
  1 error generated.

Remove this use of __counted_by to fix the warning/error. However,
rather than remove it altogether, leave it commented, as it may be
possible to support this in future compiler releases.

Cc: stable@vger.kernel.org
Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/tty/mxser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 458bb1280ebf..5b97e420a95f 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);

---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240529-drop-counted-by-ports-mxser-board-ed2a66f1a6e2

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Jiri Slaby]

On 29. 05. 24, 23:29, Nathan Chancellor wrote:
> Work for __counted_by on generic pointers in structures (not just
> flexible array members) has started landing in Clang 19 (current tip of
> tree). During the development of this feature, a restriction was added
> to __counted_by to prevent the flexible array member's element type from
> including a flexible array member itself such as:
> 
>    struct foo {
>      int count;
>      char buf[];
>    };
> 
>    struct bar {
>      int count;
>      struct foo data[] __counted_by(count);
>    };
> 
> because the size of data cannot be calculated with the standard array
> size formula:
> 
>    sizeof(struct foo) * count
> 
> This restriction was downgraded to a warning but due to CONFIG_WERROR,
> it can still break the build. The application of __counted_by on the
> ports member of 'struct mxser_board' triggers this restriction,
> resulting in:
> 
>    drivers/tty/mxser.c:291:2: error: '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.

Huh -- what am I missing:

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;
         unsigned int timeout;

         u8 read_status_mask;
         u8 ignore_status_mask;
         u8 xmit_fifo_size;

         spinlock_t slock;
};

?

>  This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>      291 |         struct mxser_port ports[] __counted_by(nports);
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.
> 
> Remove this use of __counted_by to fix the warning/error. However,
> rather than remove it altogether, leave it commented, as it may be
> possible to support this in future compiler releases.

This looks like a compiler bug/deficiency.

What does gcc say BTW?

> Cc: stable@vger.kernel.org
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
> Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")

I would not say "Fixes" here. It only works around a broken compiler.

> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>   drivers/tty/mxser.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
> index 458bb1280ebf..5b97e420a95f 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);

thanks,
-- 
js
suse labs


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Greg Kroah-Hartman]

On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
> >  This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> >      291 |         struct mxser_port ports[] __counted_by(nports);
> >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> >    1 error generated.
> > 
> > Remove this use of __counted_by to fix the warning/error. However,
> > rather than remove it altogether, leave it commented, as it may be
> > possible to support this in future compiler releases.
> 
> This looks like a compiler bug/deficiency.

I agree, why not just turn that option off in the compiler so that these
"warnings" will not show up?

> What does gcc say BTW?
> 
> > Cc: stable@vger.kernel.org
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
> > Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
> 
> I would not say "Fixes" here. It only works around a broken compiler.

Agreed, don't add Fixes: for this, it's a compiler bug, not a kernel
issue.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

On 30/05/24 09:40, Greg Kroah-Hartman wrote:
> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
>>>   This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>>>       291 |         struct mxser_port ports[] __counted_by(nports);
>>>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>     1 error generated.
>>>
>>> Remove this use of __counted_by to fix the warning/error. However,
>>> rather than remove it altogether, leave it commented, as it may be
>>> possible to support this in future compiler releases.
>>
>> This looks like a compiler bug/deficiency.
> 
> I agree, why not just turn that option off in the compiler so that these
> "warnings" will not show up?

It's not a compiler bug.

The flexible array is nested four struct layers deep, see:

ports[].port.buf.sentinel.data[]

The error report could be more specific, though.

--
Gustavo

> 
>> What does gcc say BTW?
>>
>>> Cc: stable@vger.kernel.org
>>> Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
>>> Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
>>
>> I would not say "Fixes" here. It only works around a broken compiler.
> 
> Agreed, don't add Fixes: for this, it's a compiler bug, not a kernel
> issue.
> 
> thanks,
> 
> greg k-h
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Jiri Slaby]

On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote:
> 
> 
> On 30/05/24 09:40, Greg Kroah-Hartman wrote:
>> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
>>>>   This will be an error in a future compiler version 
>>>> [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>>>>       291 |         struct mxser_port ports[] __counted_by(nports);
>>>>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>>     1 error generated.
>>>>
>>>> Remove this use of __counted_by to fix the warning/error. However,
>>>> rather than remove it altogether, leave it commented, as it may be
>>>> possible to support this in future compiler releases.
>>>
>>> This looks like a compiler bug/deficiency.
>>
>> I agree, why not just turn that option off in the compiler so that these
>> "warnings" will not show up?
> 
> It's not a compiler bug.

It is, provided the code compiles and runs.

> The flexible array is nested four struct layers deep, see:
> 
> ports[].port.buf.sentinel.data[]
> 
> The error report could be more specific, though.

Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it 
all works. The size is well known, [] is zero size, right?

Still, fix the compiler, not the code.

thanks,
-- 
js
suse labs


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Jiri Slaby]

On 30. 05. 24, 10:33, Jiri Slaby wrote:
> On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote:
>>
>>
>> On 30/05/24 09:40, Greg Kroah-Hartman wrote:
>>> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
>>>>>   This will be an error in a future compiler version 
>>>>> [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>>>>>       291 |         struct mxser_port ports[] __counted_by(nports);
>>>>>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>     1 error generated.
>>>>>
>>>>> Remove this use of __counted_by to fix the warning/error. However,
>>>>> rather than remove it altogether, leave it commented, as it may be
>>>>> possible to support this in future compiler releases.
>>>>
>>>> This looks like a compiler bug/deficiency.
>>>
>>> I agree, why not just turn that option off in the compiler so that these
>>> "warnings" will not show up?
>>
>> It's not a compiler bug.
> 
> It is, provided the code compiles and runs.
> 
>> The flexible array is nested four struct layers deep, see:
>>
>> ports[].port.buf.sentinel.data[]
>>
>> The error report could be more specific, though.
> 
> Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it 
> all works. The size is well known, [] is zero size, right?
> 
> Still, fix the compiler, not the code.

Or fix the code (properly).

Flex arrays (even empty) in the middle of structs (like 
ports[].port.buf.sentinel.data[] above is) are deprecated since gcc 14:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626516.html

So we should get rid of all those. Sooner than later.

> thanks,
-- 
-- 
js
suse labs


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Bill Wendling]

On Thu, May 30, 2024 at 1:41 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 30. 05. 24, 10:33, Jiri Slaby wrote:
> > On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote:
> >>
> >>
> >> On 30/05/24 09:40, Greg Kroah-Hartman wrote:
> >>> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
> >>>>>   This will be an error in a future compiler version
> >>>>> [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> >>>>>       291 |         struct mxser_port ports[] __counted_by(nports);
> >>>>>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>     1 error generated.
> >>>>>
> >>>>> Remove this use of __counted_by to fix the warning/error. However,
> >>>>> rather than remove it altogether, leave it commented, as it may be
> >>>>> possible to support this in future compiler releases.
> >>>>
> >>>> This looks like a compiler bug/deficiency.
> >>>
> >>> I agree, why not just turn that option off in the compiler so that these
> >>> "warnings" will not show up?
> >>
> >> It's not a compiler bug.
> >
> > It is, provided the code compiles and runs.
> >
> >> The flexible array is nested four struct layers deep, see:
> >>
> >> ports[].port.buf.sentinel.data[]
> >>
> >> The error report could be more specific, though.
> >
> > Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it
> > all works. The size is well known, [] is zero size, right?
> >
> > Still, fix the compiler, not the code.
>
> Or fix the code (properly).
>
> Flex arrays (even empty) in the middle of structs (like
> ports[].port.buf.sentinel.data[] above is) are deprecated since gcc 14:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626516.html
>
> So we should get rid of all those. Sooner than later.
>
Yes! Please do this.

-bw

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

>> So we should get rid of all those. Sooner than later.
>>
> Yes! Please do this.

Definitely, we are working on that already[1]. :)

--
Gustavo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=-Wflex-array-member-not-at-end

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Jiri Slaby]

On 30. 05. 24, 10:46, Gustavo A. R. Silva wrote:
> 
>>> So we should get rid of all those. Sooner than later.
>>>
>> Yes! Please do this.
> 
> Definitely, we are working on that already[1]. :)

FWIW undoing:
commit 7391ee16950e772076d321792d9fbf030f921345
Author: Peter Hurley <peter@hurleysoftware.com>
Date:   Sat Jun 15 09:36:07 2013 -0400

     tty: Simplify flip buffer list with 0-sized sentinel


would do the job, IMO.

thanks,
-- 
js
suse labs


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

> FWIW undoing:
> commit 7391ee16950e772076d321792d9fbf030f921345
> Author: Peter Hurley <peter@hurleysoftware.com>
> Date:   Sat Jun 15 09:36:07 2013 -0400
> 
>      tty: Simplify flip buffer list with 0-sized sentinel
> 
> 
> would do the job, IMO.

So, not even _sentinel_ is actually needed? Awesome!

I wish all of these FAMs-in-the-middle were like this one. :)

--
Gustavo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

On 6/3/24 02:26, Gustavo A. R. Silva wrote:
> 
>> FWIW undoing:
>> commit 7391ee16950e772076d321792d9fbf030f921345
>> Author: Peter Hurley <peter@hurleysoftware.com>
>> Date:   Sat Jun 15 09:36:07 2013 -0400
>>
>>      tty: Simplify flip buffer list with 0-sized sentinel
>>
>>
>> would do the job, IMO.
> 
> So, not even _sentinel_ is actually needed? Awesome!
> 

It seems that a clean revert is not possible at this point, as
the original patch is more than a decade old. If _sentinel_ is
not needed, and based on the original patch, would the following
suffice?

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 79f0ff94ce00..1b77019cc510 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -135,10 +135,7 @@ 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;
-
+       buf->tail = NULL;
         still_used = atomic_xchg(&buf->mem_used, 0);
         WARN(still_used != freed, "we still have not freed %d bytes!",
                         still_used - freed);
@@ -578,9 +575,8 @@ void tty_buffer_init(struct tty_port *port)
         struct tty_bufhead *buf = &port->buf;

         mutex_init(&buf->lock);
-       tty_buffer_reset(&buf->sentinel, 0);
-       buf->head = &buf->sentinel;
-       buf->tail = &buf->sentinel;
+       buf->head = NULL;
+       buf->tail = NULL;
         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..75fb041e43fe 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -37,7 +37,6 @@ struct tty_bufhead {
         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;

Thanks
--
Gustavo

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

> Ah, ok. The assumption is sentinel.data[] shall be unused.

This snippet is useful information.

Thanks
--
Gustavo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Nathan Chancellor]

On Thu, May 30, 2024 at 09:40:13AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote:
> > >  This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> > >      291 |         struct mxser_port ports[] __counted_by(nports);
> > >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> > >    1 error generated.
> > > 
> > > Remove this use of __counted_by to fix the warning/error. However,
> > > rather than remove it altogether, leave it commented, as it may be
> > > possible to support this in future compiler releases.
> > 
> > This looks like a compiler bug/deficiency.

I apologize. As I commented on the nvmet-fc patch, I should have
included where the flexible array member was within 'struct mxser_port',
especially since I already knew it from
https://github.com/ClangBuiltLinux/linux/issues/2026.

I hope we can all agree now that this is not a compiler bug or issue.

> I agree, why not just turn that option off in the compiler so that these
> "warnings" will not show up?

I think the only part of the warning text that got quoted above should
clarify why this is not a real solution.

For the record, if this was a true issue on the compiler side, I would
have made that very clear in the commit message (or perhaps not even
sent the patch in the first place and worked to get it fixed on the
compiler side, as ClangBuiltLinux has always tried to do first since the
beginning).

> > What does gcc say BTW?

GCC does not have any support for __counted_by merged yet. I suspect
that its current implementation won't say anything either, otherwise
Kees should have noticed it in his testing. As you and Gustavo note
further down thread, sentinel is flagged by
-Wflex-array-member-not-at-end.

  In file included from include/linux/tty_port.h:8,
                   from include/linux/tty.h:11,
                   from drivers/tty/mxser.c:24:
  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]
     40 |         struct tty_buffer sentinel;
        |                           ^~~~~~~~

Gustavo has a suggested diff to resolve the issue at the GitHub issue I
linked above that seems like a reasonable fix for both issues that is
small enough to go into stable trees that contain f34907ecca71 like this
one?

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Kees Cook]

On Wed, May 29, 2024 at 02:29:42PM -0700, Nathan Chancellor wrote:
> Work for __counted_by on generic pointers in structures (not just
> flexible array members) has started landing in Clang 19 (current tip of
> tree). During the development of this feature, a restriction was added
> to __counted_by to prevent the flexible array member's element type from
> including a flexible array member itself such as:
> 
>   struct foo {
>     int count;
>     char buf[];
>   };
> 
>   struct bar {
>     int count;
>     struct foo data[] __counted_by(count);
>   };
> 
> because the size of data cannot be calculated with the standard array
> size formula:
> 
>   sizeof(struct foo) * count
> 
> This restriction was downgraded to a warning but due to CONFIG_WERROR,
> it can still break the build. The application of __counted_by on the
> ports member of 'struct mxser_board' triggers this restriction,
> resulting in:
> 
>   drivers/tty/mxser.c:291:2: error: '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 [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>     291 |         struct mxser_port ports[] __counted_by(nports);
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>   1 error generated.
> 
> Remove this use of __counted_by to fix the warning/error. However,
> rather than remove it altogether, leave it commented, as it may be
> possible to support this in future compiler releases.
> 
> Cc: stable@vger.kernel.org
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
> Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Since this fixes a build issue under Clang, can we please land this so
v6.7 and later will build again? Gustavo is still working on the more
complete fix (which was already on his radar, so it won't be lost).

If it's easier/helpful, I can land this via the hardening tree? I was
the one who sent the bad patch originally. :)

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Gustavo A. R. Silva]

On 27/06/24 11:14, Kees Cook wrote:
> On Wed, May 29, 2024 at 02:29:42PM -0700, Nathan Chancellor wrote:
>> Work for __counted_by on generic pointers in structures (not just
>> flexible array members) has started landing in Clang 19 (current tip of
>> tree). During the development of this feature, a restriction was added
>> to __counted_by to prevent the flexible array member's element type from
>> including a flexible array member itself such as:
>>
>>    struct foo {
>>      int count;
>>      char buf[];
>>    };
>>
>>    struct bar {
>>      int count;
>>      struct foo data[] __counted_by(count);
>>    };
>>
>> because the size of data cannot be calculated with the standard array
>> size formula:
>>
>>    sizeof(struct foo) * count
>>
>> This restriction was downgraded to a warning but due to CONFIG_WERROR,
>> it can still break the build. The application of __counted_by on the
>> ports member of 'struct mxser_board' triggers this restriction,
>> resulting in:
>>
>>    drivers/tty/mxser.c:291:2: error: '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 [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
>>      291 |         struct mxser_port ports[] __counted_by(nports);
>>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>>    1 error generated.
>>
>> Remove this use of __counted_by to fix the warning/error. However,
>> rather than remove it altogether, leave it commented, as it may be
>> possible to support this in future compiler releases.
>>
>> Cc: stable@vger.kernel.org
>> Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
>> Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
>> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Since this fixes a build issue under Clang, can we please land this so
> v6.7 and later will build again? Gustavo is still working on the more
> complete fix (which was already on his radar, so it won't be lost).
> 
> If it's easier/helpful, I can land this via the hardening tree? I was
> the one who sent the bad patch originally. :)

+1 (It'd be great if you take it.)

Also, it'd be great if somebody can confirm this is an acceptable fix
for the issue:

https://lore.kernel.org/linux-hardening/c80e41e6-793e-4311-8e15-f5eda91e723e@embeddedor.com/

Thanks
--
Gustavo

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Greg Kroah-Hartman]

On Thu, Jun 27, 2024 at 10:14:05AM -0700, Kees Cook wrote:
> On Wed, May 29, 2024 at 02:29:42PM -0700, Nathan Chancellor wrote:
> > Work for __counted_by on generic pointers in structures (not just
> > flexible array members) has started landing in Clang 19 (current tip of
> > tree). During the development of this feature, a restriction was added
> > to __counted_by to prevent the flexible array member's element type from
> > including a flexible array member itself such as:
> > 
> >   struct foo {
> >     int count;
> >     char buf[];
> >   };
> > 
> >   struct bar {
> >     int count;
> >     struct foo data[] __counted_by(count);
> >   };
> > 
> > because the size of data cannot be calculated with the standard array
> > size formula:
> > 
> >   sizeof(struct foo) * count
> > 
> > This restriction was downgraded to a warning but due to CONFIG_WERROR,
> > it can still break the build. The application of __counted_by on the
> > ports member of 'struct mxser_board' triggers this restriction,
> > resulting in:
> > 
> >   drivers/tty/mxser.c:291:2: error: '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 [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
> >     291 |         struct mxser_port ports[] __counted_by(nports);
> >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
> >   1 error generated.
> > 
> > Remove this use of __counted_by to fix the warning/error. However,
> > rather than remove it altogether, leave it commented, as it may be
> > possible to support this in future compiler releases.
> > 
> > Cc: stable@vger.kernel.org
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
> > Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Since this fixes a build issue under Clang, can we please land this so
> v6.7 and later will build again? Gustavo is still working on the more
> complete fix (which was already on his radar, so it won't be lost).
> 
> If it's easier/helpful, I can land this via the hardening tree? I was
> the one who sent the bad patch originally. :)
> 

I don't see this in my queue anywhere, sorry, can you resend it?  Or
feel free to take it through your trees, no objection from me there.

thanks,

greg k-h

Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]

[Kees Cook]

On Wed, 29 May 2024 14:29:42 -0700, Nathan Chancellor wrote:
> Work for __counted_by on generic pointers in structures (not just
> flexible array members) has started landing in Clang 19 (current tip of
> tree). During the development of this feature, a restriction was added
> to __counted_by to prevent the flexible array member's element type from
> including a flexible array member itself such as:
> 
>   struct foo {
>     int count;
>     char buf[];
>   };
> 
> [...]

Applied to for-linus/hardening, thanks!

[1/1] tty: mxser: Remove __counted_by from mxser_board.ports[]
      https://git.kernel.org/kees/c/1c07c9be87dd

Take care,

-- 
Kees Cook