linux-kernel - Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2

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-next>] [day] [month] [year] [list]

Message-ID: <63aa2995-7980-430d-84be-58ce204f5172@linux.ibm.com>
Date: Mon, 18 Dec 2023 09:39:10 +0100
From: Alexandra Winter <wintera@...ux.ibm.com>
To: Wen Gu <guwen@...ux.alibaba.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 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.

>  };

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.


[...]