linux-kernel - Re: [PATCH][next] tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d19c129d-ca88-4149-80d3-12aee5c3f709@kernel.org>
Date: Wed, 5 Feb 2025 07:45:51 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] tty: tty_buffer: Avoid hundreds of
 -Wflex-array-member-not-at-end warnings

On 05. 02. 25, 6:36, Greg Kroah-Hartman wrote:
> On Wed, Feb 05, 2025 at 03:51:35PM +1030, Gustavo A. R. Silva wrote:
>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>> getting ready to enable it, globally.
>>
>> So, in order to avoid ending up with a flexible-array member in the
>> middle of other structs, we use the `struct_group_tagged()` helper
>> to create a new tagged `struct tty_buffer_hdr`. This structure
>> groups together all the members of the flexible `struct tty_buffer`
>> except the flexible array.
>>
>> As a result, the array is effectively separated from the rest of the
>> members without modifying the memory layout of the flexible structure.
>> We then change the type of the middle struct member currently causing
>> trouble from `struct tty_buffer` to `struct tty_buffer_hdr`.
>>
>> We also want to ensure that when new members need to be added to the
>> flexible structure, they are always included within the newly created
>> tagged struct. For this, we use `static_assert()`. This ensures that the
>> memory layout for both the flexible structure and the new tagged struct
>> is the same after any changes.
>>
>> This approach avoids having to implement `struct tty_buffer_hdr` as a
>> completely separate structure, thus preventing having to maintain two
>> independent but basically identical structures, closing the door to
>> potential bugs in the future.
> 
> Why not just have a separate structure and embed that in the places it
> is used?  No duplication should be needed or am I missing something?
> 
> I don't mind that, it would make this all much simpler and more obvious
> over time, and the tty layer needs all the "simplification" it can get
> :)

+100. You can name the member hdr or even h. Another approach would be 
to get rid of sentinel completely. But that might be too hard. Have you 
looked into it? You should describe that above too.

On the top of that: I remember I already looked into this when gcc14 was 
introduced and I was retracted by something else. Nevertheless, it took 
me quite a while to understand what the exact problem is and how you are 
doing the fix.

Both from the patch (the main change in tty_bufhead is hidden behind 
whitespace changes) and especially from the description (you do not say 
the simple: tty_bufhead contains data[] in the middle due to embedded 
tty_buffer there). Both need to be improved.

PS v2 was sent too early :P.

thanks,
-- 
js
suse labs