linux-kernel - Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out

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: <SJ0PR18MB400959CE08EC6BFB397CAAB3B28D9@SJ0PR18MB4009.namprd18.prod.outlook.com>
Date:   Fri, 5 Nov 2021 16:33:19 +0000
From:   "Volodymyr Mytnyk [C]" <vmytnyk@...vell.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Networking <netdev@...r.kernel.org>,
        Taras Chornyi <taras.chornyi@...ision.eu>,
        Mickey Rachamim <mickeyr@...vell.com>,
        Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
        Andrew Lunn <andrew@...n.ch>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Denis Kirjanov <dkirjanov@...e.de>,
        "Taras Chornyi [C]" <tchornyi@...vell.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "Vadym Kochan [C]" <vkochan@...vell.com>,
        Yevhen Orlov <yevhen.orlov@...ision.eu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v3] net: marvell: prestera: fix hw structure laid out

> On Thu, Nov 4, 2021 at 2:09 PM Volodymyr Mytnyk
> <volodymyr.mytnyk@...ision.eu> wrote:
> >
> > From: Volodymyr Mytnyk <vmytnyk@...vell.com>
> >
> > The prestera FW v4.0 support commit has been merged
> > accidentally w/o review comments addressed and waiting
> > for the final patch set to be uploaded. So, fix the remaining
> > comments related to structure laid out and build issues.
> >
> > Reported-by: kernel test robot <lkp@...el.com>
> > Fixes: bb5dbf2cc64d ("net: marvell: prestera: add firmware v4.0 support")
> > Signed-off-by: Volodymyr Mytnyk <vmytnyk@...vell.com>
> 
> I saw this warning today on net-next:
> 
> drivers/net/ethernet/marvell/prestera/prestera_hw.c:285:1: error:
> alignment 1 of 'union prestera_msg_port_param' is less than 4
> [-Werror=packed-not-aligned]
> 
> and this is addressed by your patch.

yes, I don't see any errors on the patchwork anymore.

> 
> However, there is still this structure that lacks explicit padding:
> 
> struct prestera_msg_acl_match {
>         __le32 type;
>         /* there is a four-byte hole on most architectures, but not on
> x86-32 or m68k */

Checked holes on x86_64 with pahole, and there is no holes. Maybe on some
other there will be. Will add 4byte member here to make sure. Thx.

>         union {
>                 struct {
>                         u8 key;
>                         u8 mask;
>                 } __packed u8;
> /* The __packed here makes no sense since this one is aligned but the

right, will remove it.

> other ones are not */
>                 struct {
>                         __le16 key;
>                         __le16 mask;
>                 } u16;
>                 struct {
>                         __le32 key;
>                         __le32 mask;
>                 } u32;
>                 struct {
>                         __le64 key;
>                         __le64 mask;
>                 } u64;
>                 struct {
>                         u8 key[ETH_ALEN];
>                         u8 mask[ETH_ALEN];
>                 } mac;
>         } keymask;
> };
> 
> and a minor issue in
> 
> struct prestera_msg_event_port_param {
>         union {
>                 struct {
>                         __le32 mode;
>                         __le32 speed;
>                         u8 oper;
>                         u8 duplex;
>                         u8 fc;
>                         u8 fec;
>                 } mac;
>                 struct {
>                         __le64 lmode_bmap;
>                         u8 mdix;
>                         u8 fc;
>                         u8 __pad[2];
>                 } __packed phy;
>         } __packed;
> } __packed;
> 
> There is no need to make the outer aggregates __packed, I would
> mark only the innermost ones here: mode, speed and lmode_bmap.
> Same for prestera_msg_port_cap_param and prestera_msg_port_param.
> 

Will add __packed only to innermost ones. Looks like only phy is required to have __packed.

> 
> It would be best to add some comments next to the __packed
> attributes to explain exactly which members are misaligned
> and why. I see that most of the packed structures are included in
> union prestera_msg_port_param, which makes that packed
> as well, however nothing that uses this union puts it on a misaligned
> address, so I don't see what the purpose is.

Will try to get rid of the __packed attributes from prestera_msg_port_param by aligning
the members in nested union of that struct. Thx.

> 
>        Arnd