| Message ID | 20240529-drop-counted-by-ports-mxser-board-v1-1-0ab217f4da6d@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | tty: mxser: Remove __counted_by from mxser_board.ports[] | expand |
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
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 >
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,
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
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,
> 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
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
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
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
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
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,
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);
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(-) --- base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0 change-id: 20240529-drop-counted-by-ports-mxser-board-ed2a66f1a6e2 Best regards,