netdev - Re: [PATCH net-next v2] net/smc: align the connect behaviour with TCP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3bb9366d-f271-a603-a280-b70ae2d59c00@linux.ibm.com>
Date:   Tue, 24 May 2022 15:05:32 +0200
From:   Karsten Graul <kgraul@...ux.ibm.com>
To:     liuyacan@...p.netease.com
Cc:     davem@...emloft.net, guangguan.wang@...ux.alibaba.com,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        pabeni@...hat.com
Subject: Re: [PATCH net-next v2] net/smc: align the connect behaviour with TCP

On 24/05/2022 14:57, liuyacan@...p.netease.com wrote:
>>>
>>>
>>> On 2022/5/23 20:24, Karsten Graul wrote:
>>>> On 13/05/2022 04:24, Guangguan Wang wrote:
>>>>> Connect with O_NONBLOCK will not be completed immediately
>>>>> and returns -EINPROGRESS. It is possible to use selector/poll
>>>>> for completion by selecting the socket for writing. After select
>>>>> indicates writability, a second connect function call will return
>>>>> 0 to indicate connected successfully as TCP does, but smc returns
>>>>> -EISCONN. Use socket state for smc to indicate connect state, which
>>>>> can help smc aligning the connect behaviour with TCP.
>>>>>
>>>>> Signed-off-by: Guangguan Wang <guangguan.wang@...ux.alibaba.com>
>>>>> Acked-by: Karsten Graul <kgraul@...ux.ibm.com>
>>>>> ---
>>>>>  net/smc/af_smc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>>  1 file changed, 46 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>>>> index fce16b9d6e1a..5f70642a8044 100644
>>>>> --- a/net/smc/af_smc.c
>>>>> +++ b/net/smc/af_smc.c
>>>>> @@ -1544,9 +1544,29 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
>>>>>  		goto out_err;
>>>>>  
>>>>>  	lock_sock(sk);
>>>>> +	switch (sock->state) {
>>>>> +	default:
>>>>> +		rc = -EINVAL;
>>>>> +		goto out;
>>>>> +	case SS_CONNECTED:
>>>>> +		rc = sk->sk_state == SMC_ACTIVE ? -EISCONN : -EINVAL;
>>>>> +		goto out;
>>>>> +	case SS_CONNECTING:
>>>>> +		if (sk->sk_state == SMC_ACTIVE)
>>>>> +			goto connected;
>>>>
>>>> I stumbled over this when thinking about the fallback processing. If for whatever reason
>>>> fallback==true during smc_connect(), the "if (smc->use_fallback)" below would set sock->state
>>>> to e.g. SS_CONNECTED. But in the fallback case sk_state keeps SMC_INIT. So during the next call
>>>> the SS_CONNECTING case above would break because sk_state in NOT SMC_ACTIVE, and we would end
>>>> up calling kernel_connect() again. Which seems to be no problem when kernel_connect() returns 
>>>> -EISCONN and we return this to the caller. But is this how it should work, or does it work by chance?
>>>>
>>>
>>> Since the sk_state keeps SMC_INIT and does not correctly indicate the state of clcsock, it should end
>>> up calling kernel_connect() again to get the actual connection state of clcsock.
>>>
>>> And I'm sorry there is a problem that if sock->state==SS_CONNECTED and sk_state==SMC_INIT, further call
>>> of smc_connect will return -EINVAL where -EISCONN is preferred. 
>>> The steps to reproduce:
>>> 1）switch fallback before connect, such as setsockopt TCP_FASTOPEN
>>> 2）connect with noblocking and returns -EINPROGRESS. (sock->state changes to SS_CONNECTING)
>>> 3) end up calling connect with noblocking again and returns 0. (kernel_connect() returns 0 and sock->state changes to
>>>    SS_CONNECTED but sk->sk_state stays SMC_INIT)
>>> 4) call connect again, maybe by mistake, will return -EINVAL, but -EISCONN is preferred.
>>>
>>> What do you think about if we synchronize the sk_state to SMC_ACTIVE instead of keeping SMC_INIT when clcsock
>>> connected successfully in fallback case described above.
>>>
>>> ...
>>
>> I start thinking that the fix in 86434744 introduced a problem. Before that fix a connect with
>> fallback always reached __smc_connect() and on top of that function in case of fallback
>> smc_connect_fallback() is called, which itself sets sk_state to SMC_ACTIVE.
>>
>> 86434744 removed that code path and I wonder what it actually fixed, because at this time the 
>> fallback check in __smc_connect() was already present.
>>
>> Without that "goto out;" the state would be set correctly in smc_connect_fallback(), and the 
>> socket close processing would work as expected.
> 
> I think it is OK without that "goto out;". And I guess the purpose of "goto out;" is to avoid calling __smc_connect(), 
> because it is impossible to establish an rdma channel at this time.

Yes that was the purpose, but this disabled all the extra processing that should be done
for fallback sockets during connect().