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: <38f06cfb-2d68-2b10-f82b-62a44c25b8f8@linux.alibaba.com>
Date: Wed, 20 Dec 2023 20:16:50 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>, wenjia@...ux.ibm.com,
 hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, kgraul@...ux.ibm.com, jaka@...ux.ibm.com
Cc: borntraeger@...ux.ibm.com, svens@...ux.ibm.com,
 alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com, raspl@...ux.ibm.com,
 schnelle@...ux.ibm.com, guangguan.wang@...ux.alibaba.com,
 linux-s390@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v8 03/10] net/smc: unify the structs of accept or
 confirm message for v1 and v2



On 2023/12/20 19:37, Alexandra Winter wrote:
> 
> 
> On 19.12.23 15:26, Wen Gu wrote:
>>   struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
>> -	struct smc_clc_msg_hdr hdr;
>> -	union {
>> -		struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
>> -		struct { /* SMC-D */
>> -			struct smcd_clc_msg_accept_confirm_common d0;
>> -			u32 reserved5[3];
>> -		};
>> -	};
>> -} __packed;			/* format defined in RFC7609 */
>> -
>> -struct smc_clc_msg_accept_confirm_v2 {	/* clc accept / confirm message */
>>   	struct smc_clc_msg_hdr hdr;
>>   	union {
>>   		struct { /* SMC-R */
>>   			struct smcr_clc_msg_accept_confirm r0;
>> -			u8 eid[SMC_MAX_EID_LEN];
>> -			u8 reserved6[8];
>> -		} r1;
>> +			struct { /* v2 only */
>> +				u8 eid[SMC_MAX_EID_LEN];
>> +				u8 reserved6[8];
>> +			} __packed r1;
>> +		};
>>   		struct { /* SMC-D */
>>   			struct smcd_clc_msg_accept_confirm_common d0;
>> -			__be16 chid;
>> -			u8 eid[SMC_MAX_EID_LEN];
>> -			u8 reserved5[8];
>> -		} d1;
>> +			struct { /* v2 only, but 12 bytes reserved in v1 */
>> +				__be16 chid;
>> +				u8 eid[SMC_MAX_EID_LEN];
>> +				u8 reserved5[8];
>> +			} __packed d1;
>> +		};
>>   	};
>>   };
> 
> 
> I still think the __packed at the outmost level is the safest place.
> Like you have it now the compiler could place unused memory between
> ro and r1 or between d0 and d1.
> Afaik compilers don't do that, if the blocks are word-aligned, but
> there is no guarantee.
> 
> Up to you. My R-b still applies.
> Sandy

Thank you, Sandy.

IIUC, if only outmost level has __packed, it won't work for the inner block.

e.g.

If __packed is added at d1 and r1:

struct smc_clc_msg_accept_confirm {     /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
                 struct { /* SMC-R */
                         struct smcr_clc_msg_accept_confirm r0;
                         struct { /* v2 only */
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u8 reserved6[8];
                         } __packed r1;
                 };
                 struct { /* SMC-D */
                         struct smcd_clc_msg_accept_confirm_common d0;
                         struct { /* v2 only, but 12 bytes reserved in v1 */
                                 __be16 chid;
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u64 gid_ext;
                         } __packed d1;
                 };
         };
};

According to pahole, it will be:

struct smc_clc_msg_accept_confirm {
         struct smc_clc_msg_hdr     hdr;                  /*     0     8 */
         union {
                 struct {
                         struct smcr_clc_msg_accept_confirm r0; /*     8    56 */
                         /* --- cacheline 1 boundary (64 bytes) --- */
                         struct {
                                 u8 eid[32];              /*    64    32 */
                                 u8 reserved6[8];         /*    96     8 */
                         } r1;                            /*    64    40 */
                 };                                       /*     8    96 */
                 struct {
                         struct smcd_clc_msg_accept_confirm_common d0; /*     8    24 */
                         struct {
                                 __be16 chid;             /*    32     2 */
                                 u8 eid[32];              /*    34    32 */
                                 /* --- cacheline 1 boundary (64 bytes) was 2 bytes ago --- */
                                 u64 gid_ext;             /*    66     8 */
                         } __attribute__((__packed__)) d1; /*    32    42 */
                 } __attribute__((__packed__));           /*     8    66 */
         };                                               /*     8    96 */

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


If __packed is added at outmost level:

struct smc_clc_msg_accept_confirm {     /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
                 struct { /* SMC-R */
                         struct smcr_clc_msg_accept_confirm r0;
                         struct { /* v2 only */
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u8 reserved6[8];
                         } r1;
                 };
                 struct { /* SMC-D */
                         struct smcd_clc_msg_accept_confirm_common d0;
                         struct { /* v2 only, but 12 bytes reserved in v1 */
                                 __be16 chid;
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u64 gid_ext;
                         } d1;
                 };
         };
} __packed;

According to pahole, it will be:

struct smc_clc_msg_accept_confirm {
         struct smc_clc_msg_hdr     hdr;                  /*     0     8 */
         union {
                 struct {
                         struct smcr_clc_msg_accept_confirm r0; /*     8    56 */
                         /* --- cacheline 1 boundary (64 bytes) --- */
                         struct {
                                 u8 eid[32];              /*    64    32 */
                                 u8 reserved6[8];         /*    96     8 */
                         } r1;                            /*    64    40 */
                 };                                       /*     8    96 */
                 struct {
                         struct smcd_clc_msg_accept_confirm_common d0; /*     8    24 */
                         struct {
                                 __be16 chid;             /*    32     2 */
                                 u8 eid[32];              /*    34    32 */

                                 /* XXX 6 bytes hole, try to pack */

                                 /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
                                 u64 gid_ext;             /*    72     8 */
                         } d1;                            /*    32    48 */   <- doesn't work for inner d1.
                 };                                       /*     8    72 */
         };                                               /*     8    96 */

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


I also considered add them all:

struct smc_clc_msg_accept_confirm {     /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
                 struct { /* SMC-R */
                         struct smcr_clc_msg_accept_confirm r0;
                         struct { /* v2 only */
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u8 reserved6[8];
                         } __packed r1;
                 } __packed;
                 struct { /* SMC-D */
                         struct smcd_clc_msg_accept_confirm_common d0;
                         struct { /* v2 only, but 12 bytes reserved in v1 */
                                 __be16 chid;
                                 u8 eid[SMC_MAX_EID_LEN];
                                 u64 gid_ext;
                         } __packed d1;
                 } __packed;
         };
} __packed;

but a little bit strange since for only d1 needs to packed, so I kept it as it is now.

Thanks,
Wen Gu

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ