linux-kernel - Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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]

Date:   Tue, 10 Jan 2023 14:02:52 +0200
From:   Kalle Valo <kvalo@...nel.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Martin Blumenstingl' <martin.blumenstingl@...glemail.com>,
        Ping-Ke Shih <pkshih@...ltek.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@...r.kernel.org>,
        "tehuang\@realtek.com" <tehuang@...ltek.com>,
        "s.hauer\@pengutronix.de" <s.hauer@...gutronix.de>,
        "tony0620emma\@gmail.com" <tony0620emma@...il.com>,
        "netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

David Laight <David.Laight@...LAB.COM> writes:

> From: Martin Blumenstingl
>> Sent: 04 January 2023 15:30
>> 
>> Hi Ping-Ke, Hi David,
>> 
>> On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@...ltek.com> wrote:
>> [...]
>> > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as
>> I think this can be done in a separate patch.
>> My v2 of this patch has reduced these changes to a minimum, see [0]
>> 
>> [...]
>> > struct rtw8821ce_efuse {
>> >    ...
>> >    u8 data1;       // offset 0x100
>> >    __le16 data2;   // offset 0x101-0x102
>> >    ...
>> > } __packed;
>> >
>> > Without __packed, compiler could has pad between data1 and data2,
>> > and then get wrong result.
>> My understanding is that this is the reason why we need __packed.
>
> True, but does it really have to look like that?
> I can't find that version (I don't have a net_next tree).
> Possibly it should be 'u8 data2[2];'
>
> Most hardware definitions align everything.
>
> What you may want to do is add compile-time asserts for the
> sizes of the structures.
>
> Remember that if you have 16/32 bit fields in packed structures
> on some architectures the compile has to generate code that does
> byte loads and shifts.
>
> The 'misaligned' property is lost when you take the address - so
> you can easily generate a fault.
>
> Adding __packed to a struct is a sledgehammer you really shouldn't need.

Avoiding use of __packed is news to me, but is this really a safe rule?
Most of the wireless engineers are no compiler experts (myself included)
so I'm worried. For example, in ath10k and ath11k I try to use __packed
for all structs which are accessing hardware or firmware just to make
sure that the compiler is not changing anything.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches