Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Wenjia Zhang <wenjia@linux.ibm.com>]

On 06.12.24 11:51, Wenjia Zhang wrote:
> 
> 
> On 06.12.24 07:06, Guangguan Wang wrote:
>>
>>
>> On 2024/12/5 20:58, Halil Pasic wrote:
>>> On Thu, 5 Dec 2024 11:16:27 +0100
>>> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
>>>
>>>>> --- a/net/smc/af_smc.c
>>>>> +++ b/net/smc/af_smc.c
>>>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>>>        ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>>>        if (!(ini->smcr_version & SMC_V2) ||
>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>> +        (smc->clcsock->sk->sk_family != AF_INET &&
>>>>> +
>>>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>>>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>>>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>>>> i.e. here should be:
>>>> （smc->clcsock->sk->sk_family != AF_INET）||
>>>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
>>>
>>> Wenjia, I think you happen to confuse something here. The condition
>>> of this if statement is supposed to evaluate as true iff we don't want
>>> to propose SMCRv2 because the situation is such that SMCRv2 is not
>>> supported.
>>>
>>> We have a bunch of conditions we need to meet for SMCRv2 so
>>> logically we have (A && B && C && D). Now since the if is
>>> about when SMCRv2 is not supported we have a super structure
>>> that looks like !A || !B || !C || !D. With this patch, if
>>> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
>>> if smc->clcsock->sk->sk_family is something else that AF_INET
>>> the we do not do SMCRv2!
>>>
>>> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
>>> AF_INET6 sockets too if the addresses used are actually
>>> v4 mapped addresses.
>>>
>>> Now this is where the cognitive dissonance starts on my end. I
>>> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
>>> is a tautology in this context. That may be a reasonable thing to
>>> assume. Under that assumption
>>> sk_family != AF_INET &&    !ipv6_addr_v4mapped(addr) (shortened for
>>> convenience)
>>> becomes equivalent to
>>> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
>>> which means in words if the socket is an IPv6 sockeet and the addr is 
>>> not
>>> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
>>> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
>>> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
>>> the aforementioned assumption.
>>
>> Hi, Halil
>>
>> Thank you for such a detailed derivation.
>>
>> Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. 
>> Indeed,
>> many codes in SMC have already made this assumption, for example,
>> static int __smc_create(struct net *net, struct socket *sock, int 
>> protocol,
>>             int kern, struct socket *clcsock)
>> {
>>     int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
>>     ...
>> }
>> And I also believe it is reasonable.
>>
>> Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. 
>> This patch
>> introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is 
>> equivalen
>> to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.
>>
>>>
>>> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
>>> the #else does not make any sense, because I guess with IPv6 not
>>> available AF_INET6 is not available ant thus the else is always
>>> guaranteed to evaluate to false under the assumption made.
>>>
>> You are right. The #else here does not make any sense. It's my mistake.
>>
>> The condition is easier to understand and read should be like this:
>>       if (!(ini->smcr_version & SMC_V2) ||
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>> +#endif
>>           !smc_clc_ueid_count() ||
>>           smc_find_rdma_device(smc, ini))
>>           ini->smcr_version &= ~SMC_V2;
>>
> 
> sorry, I still don't agree on this version. You removed the condition
> "
> smc->clcsock->sk->sk_family != AF_INET ||
> "
> completely. What about the socket with neither AF_INET nor AF_INET6 family?
> 
> Thanks,
> Wenjia
> 
I think the main problem in the original version was that
(sk_family != AF_INET) is not equivalent to (sk_family == AF_INET6).
Since you already in the new version above used sk_family == AF_INET6,
the else condition could stay as it is. My suggestion:

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 8e3093938cd2..5f205a41fc48 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct 
smc_sock *smc,
         ini->check_smcrv2 = true;
         ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
         if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+           (smc->clcsock->sk->sk_family == AF_INET6 &&
+            !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
+#else
             smc->clcsock->sk->sk_family != AF_INET ||
+#endif
             !smc_clc_ueid_count() ||
             smc_find_rdma_device(smc, ini))
                 ini->smcr_version &= ~SMC_V2;

Thanks,
Wenjia