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 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