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] [day] [month] [year] [list]
Message-ID:
 <DM6PR12MB45163B5A12A98382D359BBF6D80A2@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Sun, 14 Apr 2024 08:20:44 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Russell King <linux@...linux.org.uk>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"corbet@....net" <corbet@....net>, "sdf@...gle.com" <sdf@...gle.com>,
	"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
	"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
	"vladimir.oltean@....com" <vladimir.oltean@....com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "shayagr@...zon.com" <shayagr@...zon.com>,
	"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>, "jiri@...nulli.us"
	<jiri@...nulli.us>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, mlxsw
	<mlxsw@...dia.com>, Petr Machata <petrm@...dia.com>, Ido Schimmel
	<idosch@...dia.com>
Subject: RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands

> -----Original Message-----
> From: Danielle Ratson <danieller@...dia.com>
> Sent: Tuesday, 9 April 2024 16:34
> To: Russell King <linux@...linux.org.uk>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; corbet@....net; sdf@...gle.com;
> kory.maincent@...tlin.com; maxime.chevallier@...tlin.com;
> vladimir.oltean@....com; przemyslaw.kitszel@...el.com;
> ahmed.zaki@...el.com; richardcochran@...il.com; shayagr@...zon.com;
> paul.greenwalt@...el.com; jiri@...nulli.us; linux-doc@...r.kernel.org; linux-
> kernel@...r.kernel.org; mlxsw <mlxsw@...dia.com>; Petr Machata
> <petrm@...dia.com>; Ido Schimmel <idosch@...dia.com>
> Subject: RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
> 
> > -----Original Message-----
> > From: Russell King <linux@...linux.org.uk>
> > Sent: Monday, 8 April 2024 17:55
> > To: Danielle Ratson <danieller@...dia.com>
> > Cc: netdev@...r.kernel.org; davem@...emloft.net;
> edumazet@...gle.com;
> > kuba@...nel.org; pabeni@...hat.com; corbet@....net; sdf@...gle.com;
> > kory.maincent@...tlin.com; maxime.chevallier@...tlin.com;
> > vladimir.oltean@....com; przemyslaw.kitszel@...el.com;
> > ahmed.zaki@...el.com; richardcochran@...il.com; shayagr@...zon.com;
> > paul.greenwalt@...el.com; jiri@...nulli.us; linux-doc@...r.kernel.org;
> > linux- kernel@...r.kernel.org; mlxsw <mlxsw@...dia.com>; Petr Machata
> > <petrm@...dia.com>; Ido Schimmel <idosch@...dia.com>
> > Subject: Re: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> > supporting CDB commands
> >
> > On Mon, Apr 08, 2024 at 03:53:37PM +0300, Danielle Ratson wrote:
> > > +/**
> > > + * struct ethtool_cmis_cdb_request - CDB commands request fields as
> > decribed in
> > > + *				the CMIS standard
> > > + * @id: Command ID.
> > > + * @epl_len: EPL memory length.
> > > + * @lpl_len: LPL memory length.
> > > + * @chk_code: Check code for the previous field and the payload.
> > > + * @resv1: Added to match the CMIS standard request continuity.
> > > + * @resv2: Added to match the CMIS standard request continuity.
> > > + * @payload: Payload for the CDB commands.
> > > + */
> > > +struct ethtool_cmis_cdb_request {
> > > +	__be16 id;
> > > +	struct_group(body,
> > > +		u16 epl_len;
> >
> > u16 with a struct that also uses __be16 looks suspicious.
> >
> > > +		u8 lpl_len;
> > > +		u8 chk_code;
> > > +		u8 resv1;
> > > +		u8 resv2;
> > > +		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
> > > +	);
> >
> > Does it matter if the compiler inserts some padding before this struct group?
> 
> Yes it should matter since I copy this struct into a payload and it should be
> transferred like that to match the CMIS specification, but if I use __be16 for
> both id and epl_len it might resolve the issue isn’t it?
> 
> >
> > > +/**
> > > + * struct ethtool_cmis_cdb_rpl_hdr - CDB commands reply header
> > > +arguments
> > > + * @rpl_len: Reply length.
> > > + * @rpl_chk_code: Reply check code.
> > > + */
> > > +struct ethtool_cmis_cdb_rpl_hdr {
> > > +	u8 rpl_len;
> > > +	u8 rpl_chk_code;
> >
> > Does it matter if the compiler adds some padding here?
> 
> Kind of the same idea, it is matter if the compiler adds padding since the reply
> is read and extracted into this struct like it is written in the CMIS specification.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Hi Russell, 

Please see below pahole's output after changing ethtool_cmis_cdb_request ::epl_len from u16 to __be16.

$ pahole -C 'ethtool_cmis_cdb_request' ./vmlinux
struct ethtool_cmis_cdb_request {
        __be16                     id;                   /*     0     2 */
        union {
                struct {
                        __be16     epl_len;              /*     2     2 */
                        u8         lpl_len;              /*     4     1 */
                        u8         chk_code;             /*     5     1 */
                        u8         resv1;                /*     6     1 */
                        u8         resv2;                /*     7     1 */
                        u8         payload[120];         /*     8   120 */
                };                                       /*     2   126 */
                struct {
                        __be16     epl_len;              /*     2     2 */
                        u8         lpl_len;              /*     4     1 */
                        u8         chk_code;             /*     5     1 */
                        u8         resv1;                /*     6     1 */
                        u8         resv2;                /*     7     1 */
                        u8         payload[120];         /*     8   120 */
                } body;                                  /*     2   126 */
        };                                               /*     2   126 */

        /* size: 128, cachelines: 2, members: 2 */
};

[danieller@...-r-vrt-155 linux_mlxsw]$ pahole -C 'ethtool_cmis_cdb_rpl' ./vmlinux
struct ethtool_cmis_cdb_rpl {
        struct ethtool_cmis_cdb_rpl_hdr hdr;             /*     0     2 */
        u8                         payload[120];         /*     2   120 */

        /* size: 122, cachelines: 2, members: 2 */
        /* last cacheline: 58 bytes */
};

[danieller@...-r-vrt-155 linux_mlxsw]$ pahole -C 'ethtool_cmis_cdb_rpl_hdr' ./vmlinux
struct ethtool_cmis_cdb_rpl_hdr {
        u8                         rpl_len;              /*     0     1 */
        u8                         rpl_chk_code;         /*     1     1 */

        /* size: 2, cachelines: 1, members: 2 */
        /* last cacheline: 2 bytes */
};

If a further fix is needed, you are welcome to let me know.

Thanks,
Danielle

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ