| 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
| ||
|
Message-ID: <868f5d66-ac74-4b0a-a0d0-e44fdea3bb73@linux.ibm.com>
Date: Mon, 9 Dec 2024 09:49:23 +0100
From: Wenjia Zhang <wenjia@...ux.ibm.com>
To: Guangguan Wang <guangguan.wang@...ux.alibaba.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 09.12.24 07:04, Guangguan Wang wrote:
>
>
> 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?
>
The answer is no, at least in the near future. As you might be already
aware, it depends on the implementation on z/OS.
> 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;
>
Ok, I got your point, a socket with an address family other than AF_INET
and AF_INET6 is already pre-filtered, so that such extra condition
checking for the smc->clcsock->sk->sk_family != AF_INET is not
necessary, right?
Would you like to send a new version? And feel free to use this in the
new version:
Reviewed-by: Wenjia Zhang <wenjia@...ux.ibm.com>
Thanks,
Wenjia
Powered by blists - more mailing lists