| 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
| ||
|
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