linux-kernel - Re: [PATCH net-next v5 2/9] net/smc: introduce sub-functions for smc_clc_send_confirm

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: <7ff3001a-4254-0382-f8c8-6ebf2807346a@linux.alibaba.com>
Date:   Mon, 11 Dec 2023 23:23:57 +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 v5 2/9] net/smc: introduce sub-functions for
 smc_clc_send_confirm_accept()



On 2023/12/11 21:35, Alexandra Winter wrote:
> 
> 
> On 11.12.23 13:15, Wen Gu wrote:
>>>> +    clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
>>>
>>> Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept
>>> and in smc_clc_send_confirm_accept)
>>> smc_clc_msg_accept_confirm_v2 has hdr and d0 as well.
>>
>> I think the cast is to imply that v2 is an expansion of v1, or v1 is the base of v2.
>> So here using clc(v1) reperesents their common set.
>>
>> If we use smc_clc_msg_accept_confirm_v2 for all, I think readers may be tempted to
>> check whether the hdr and d0 in 'smc_clc_msg_accept_confirm_v2' are also applicable to v1.
>>
>> And there are settings below that are specific for v1. It may be confusing if we
>> change it like this:
>>
>> if (version == SMC_V1) {
>>      clc_v2->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
>> } else {
>>
>>
>>>
>>> IMO, it would be a nice seperate patch to get rid of the 2 type defs for
>>> smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2
>>> and all the related casting anyhow.
>>>
>>
>> Do you mean to define only smc_clc_msg_accept_confirm_v2 or define with the name
>> of smc_clc_msg_accept_confirm but the contents of smc_clc_msg_accept_confirm_v2?
>>
>> I have a different opinion on this, since I think the smc_clc_msg_accept_confirm
>> and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and
>> v2 messages and remind people what is currently working on. So I perfer to keep them.
>> Am I missing something?
>>
> 
> 
> This is a discussion about coding style, readability and maintainability (avoid future errors).
> And the code works today and the rest is opinions. That said, let me list some arguments why
> I don't like the casts.
> 
> Casts in general break the type checking of the compiler.
> 
> In some places e.g. clc.d0 points to struct smc_clc_msg_accept_confirm in other
> places it points to struct smc_clc_msg_accept_confirm_v2.
> This makes it hard to find all places where e.g. d0 is altered. (e.g. with an IDE).
> 
> You say: "smc_clc_msg_accept_confirm
>> and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and
>> v2 messages"
> But that is not even the case in the code that this patch changes:
> In smcd_clc_prep_confirm_accept() you pass a struct smc_clc_msg_accept_confirm_v2
> cast it to v1 (even in the v2 case) and then use the v1 layout for the common fields and
> the v1-only fields. So I don't think that helps very much.
> 
> The v2 messages were explicitely defined for compatibility. i.e.
> all v1 fields are still available. It would be good to see that in the code as well.
> With 2 differnet structs you don't emphasize that.
> 
> With future changes somebody could easily make a mistake that the 2 structures don't
> have the same size anymore. And then the casting can lead to out-of-bound error that
> are hard to find.
> 
> We want v2 to be the usual case and v1 to be the exception for backwards compatibility.
> FOr historic reasons, the code looks as if v2 is the exception. I'd rather point out the
> remaining v1 cases.
> 
> 
> 
> I could envision something like:
> 
> 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];
> 			__be64 gid_ext;
> 		} __packed d1;
> 	};
> };
> 
> And then only use this one structure.
> 

Thank you Sandy for the detailed explanation.

What I considered, as mentioned above, is that if the two are combined,
it may be difficult to distinguish according to the name what situation
I am in, v1 or v2?

But I do agree with your concern about the potential errors that caused
by future divergence of the two struct if they are defined separately.

I will try to combine them into one struct in a seperate patch.

Thank you.