Message-ID: <d2af79e2-adb2-46f0-a7e3-67a9265f3adf@linux.alibaba.com>
Date: Mon, 9 Dec 2024 14:04:40 +0800
From: Guangguan Wang <guangguan.wang@...ux.alibaba.com>
To: Wenjia Zhang <wenjia@...ux.ibm.com>, Halil Pasic <pasic@...ux.ibm.com>
Cc: jaka@...ux.ibm.com, alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
 guwen@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
 linux-rdma@...r.kernel.org, linux-s390@...r.kernel.org,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Dust Li <dust.li@...ux.alibaba.com>
Subject: Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr
 client for smc-r v2



On 2024/12/7 03:49, Wenjia Zhang wrote:
> 
> 
> 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@...ux.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

The RFC7609 have confined SMC to socket applications using stream (i.e., TCP) sockets over IPv4 or IPv6.
https://datatracker.ietf.org/doc/html/rfc7609#page-26:~:text=It%20is%20confined%20to%20socket%20applications%20using%20stream%0A%20%20%20(i.e.%2C%20TCP)%20sockets%20over%20IPv4%20or%20IPv6

Both in the smc-tools and in smc kernel module, we can see codes that the sk_family is either AF_INET or AF_INET6.
The codes here:
https://raw.githubusercontent.com/ibm-s390-linux/smc-tools/refs/heads/main/smc-preload.c#:~:text=if%20((domain%20%3D%3D%20AF_INET%20%7C%7C%20domain%20%3D%3D%20AF_INET6)%20%26%26
and
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=(sk%2D%3Esk_family%20!%3D%20AF_INET%20%26%26%20sk%2D%3Esk_family%20!%3D%20AF_INET6))
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=int%20family%20%3D%20(protocol%20%3D%3D%20SMCPROTO_SMC6)%20%3F%20PF_INET6%20%3A%20PF_INET%3B 
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esin_family%20!%3D-,AF_INET,-%26%26%0A%09%20%20%20%20addr%2D%3E
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esa_family%20!%3D-,AF_INET6,-)%0A%09%09goto%20out_err
...

I wonder if SMC-R can support other address famliy rather than AF_INET AF_INET6 in design？
And IBM has any plan to support other address family in future?  Wenjia, can you help explain
this?

If the answer is positive, the code should be like this:
        if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+           !(smc->clcsock->sk->sk_family == AF_INET || (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;

Otherwise, the code below is reasonable.
      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;

Thanks,
Guangguan Wang

Powered by blists - more mailing lists