| 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: Mon, 18 Dec 2023 20:21:28 +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 v6 03/10] net/smc: unify the structs of accept or
confirm message for v1 and v2
On 2023/12/18 16:39, Alexandra Winter wrote:
>
>
> On 12.12.23 09:52, Wen Gu wrote:
>> The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
>> separately defined and often casted to each other in the code, which may
>> increase the risk of errors caused by future divergence of them. So
>> unify them into one struct for better maintainability.
>>
>> Suggested-by: Alexandra Winter <wintera@...ux.ibm.com>
>> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
>> ---
>> net/smc/af_smc.c | 50 +++++++++++++++---------------------------
>> net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
>> net/smc/smc_clc.h | 32 ++++++++++-----------------
>> 3 files changed, 57 insertions(+), 90 deletions(-)
>>
>
> [...]
> Thank you very much, Wen Gu. I think this makes it much easier to spot the
> places in the accept/confirm code code where v1 vs v2 really make a difference.
> I understand that this is not really related to v2.1, but I feel it is worth
> simplifying the already complex strucutres before adding even more complexity.
>
>
>
>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
>> index 1697b84..614fa2f 100644
>> --- a/net/smc/smc_clc.h
>> +++ b/net/smc/smc_clc.h
>> @@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
>> 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;
>> + struct smcr_clc_msg_accept_confirm _r0;
>> + /* v2 only, reserved and ignored in v1: */
>> u8 eid[SMC_MAX_EID_LEN];
>> u8 reserved6[8];
>> } r1;
>> struct { /* SMC-D */
>> - struct smcd_clc_msg_accept_confirm_common d0;
>> + struct smcd_clc_msg_accept_confirm_common _d0;
>> + /* v2 only, reserved and ignored in v1: */
>> __be16 chid;
>> u8 eid[SMC_MAX_EID_LEN];
>> u8 reserved5[8];
>> } d1;
>> };
>> +#define r0 r1._r0
>> +#define d0 d1._d0
>
> This adds complexity.
> If you add the v2-only fields to struct smcr_clc_msg_accept_confirm and
> struct smcd_clc_msg_accept_confirm_common respectively, you can avoid the
> #define and the extra layer in the struct.
> Actually there are already v2-only fields in smcd_clc_msg_accept_confirm_common
> and smcd_clc_msg_accept_confirm_common (gid and others). So you could add the
> correct informative comments there.
Thank you very much for the suggestions, Sandy.
I checked the history commits:
c758dfddc1b5 ("net/smc: add SMC-D support in CLC messages")
3d9725a6a133 ("net/smc: common routine for CLC accept and confirm")
a7c9c5f4af7f ("net/smc: CLC accept / confirm V2")
e5c4744cfb59 ("net/smc: add SMC-Rv2 connection establishment")
The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
seem to have not changed since SMCDv1. So I guess there is no v2-only fields
in this two struct. I tried to confirm this in some documents but didn't find
the message format for v1.
If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
is inherited from v1, should we still put the fields of v2 into these two structures?
If still, I will change these structures as
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 614fa2f298f5..18157aeb14ec 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm { /* SMCR accept/confirm */
__be64 rmb_dma_addr; /* RMB virtual address */
u8 reserved2;
u8 psn[3]; /* packet sequence number */
+ /* v2 only, reserved and ignored in v1: */
+ u8 eid[SMC_MAX_EID_LEN];
+ u8 reserved6[8];
} __packed;
-struct smcd_clc_msg_accept_confirm_common { /* SMCD accept/confirm */
+struct smcd_clc_msg_accept_confirm { /* SMCD accept/confirm */
__be64 gid; /* Sender GID */
__be64 token; /* DMB token */
u8 dmbe_idx; /* DMBE index */
@@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common { /* SMCD accept/confirm */
#endif
u16 reserved4;
__be32 linkid; /* Link identifier */
+ /* v2 only, reserved and ignored in v1: */
+ __be16 chid;
+ u8 eid[SMC_MAX_EID_LEN];
+ u8 reserved5[8];
} __packed;
#define SMC_CLC_OS_ZOS 1
@@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext {
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;
- /* v2 only, reserved and ignored in v1: */
- u8 eid[SMC_MAX_EID_LEN];
- u8 reserved6[8];
- } r1;
- struct { /* SMC-D */
- struct smcd_clc_msg_accept_confirm_common _d0;
- /* v2 only, reserved and ignored in v1: */
- __be16 chid;
- u8 eid[SMC_MAX_EID_LEN];
- u8 reserved5[8];
- } d1;
+ struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
+ struct smcd_clc_msg_accept_confirm d0; /* SMC-D */
};
-#define r0 r1._r0
-#define d0 d1._d0
};
>
>> };
>
> You have removed the __packed attribute.
> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
>
r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in
this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext,
thus making a hole before gid_ext, so I added __packed attribute to SMC-D.
If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R.
Thanks.
>
> [...]
Powered by blists - more mailing lists