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]

Date:   Tue, 24 May 2022 10:59:46 +0800
From:   Guangguan Wang <guangguan.wang@...ux.alibaba.com>
To:     Karsten Graul <kgraul@...ux.ibm.com>, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com
Cc:     linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net/smc: align the connect behaviour with TCP



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.

...
if (smc->use_fallback) {
	sock->state = rc ? SS_CONNECTING : SS_CONNECTED;
	if (!rc)
		sk->sk_state = SMC_ACTIVE;    /* synchronize sk_state from SMC_INIT to SMC_ACTIVE */
	goto out;
}
...

>> +		break;
>> +	case SS_UNCONNECTED:
>> +		sock->state = SS_CONNECTING;
>> +		break;
>> +	}
>> +
>>  	switch (sk->sk_state) {
>>  	default:
>>  		goto out;
>> +	case SMC_CLOSED:
>> +		rc = sock_error(sk) ? : -ECONNABORTED;
>> +		sock->state = SS_UNCONNECTED;
>> +		goto out;
>>  	case SMC_ACTIVE:
>>  		rc = -EISCONN;
>>  		goto out;
>> @@ -1565,20 +1585,24 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
>>  		goto out;
>>  
>>  	sock_hold(&smc->sk); /* sock put in passive closing */
>> -	if (smc->use_fallback)
>> +	if (smc->use_fallback) {
>> +		sock->state = rc ? SS_CONNECTING : SS_CONNECTED;
>>  		goto out;
>> +	}
>>  	if (flags & O_NONBLOCK) {
>>  		if (queue_work(smc_hs_wq, &smc->connect_work))
>>  			smc->connect_nonblock = 1;
>>  		rc = -EINPROGRESS;
>> +		goto out;
>>  	} else {
>>  		rc = __smc_connect(smc);
>>  		if (rc < 0)
>>  			goto out;
>> -		else
>> -			rc = 0; /* success cases including fallback */
>>  	}
>>  
>> +connected:
>> +	rc = 0;
>> +	sock->state = SS_CONNECTED;
>>  out:
>>  	release_sock(sk);
>>  out_err: