Re: [PATCH net] net/smc: fix fallback failed while sendmsg with fastopen

["D. Wythe" <alibuda@linux.alibaba.com>]

Hi Simon,

Thank you for your suggestion.  Your writing style is more elegant.

I will modify it according to your plan. Can I add your name as a 
co-developer?

Best wishes.

D. Wythe

On 3/6/23 8:24 PM, Simon Horman wrote:
> On Mon, Mar 06, 2023 at 12:08:48PM +0800, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> Before determining whether the msg has unsupported options, it has been
>> prematurely terminated by the wrong status check.
>>
>> For the application, the general method of MSG_FASTOPEN likes
>>
>> fd = socket(...)
>> /* rather than connect */
>> sendto(fd, data, len, MSG_FASTOPEN)
>>
>> Hence, We need to check the flag before state check, because the sock state
>> here is always SMC_INIT when applications tries MSG_FASTOPEN. Once we
>> found unsupported options, fallback it to TCP.
>>
>> Fixes: ee9dfbef02d1 ("net/smc: handle sockopts forcing fallback")
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c | 26 ++++++++++++++++----------
>>   1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index b233c94..fd80879 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -2662,24 +2662,30 @@ static int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>>   	int rc = -EPIPE;
>>   
>>   	smc = smc_sk(sk);
>> -	lock_sock(sk);
>> -	if ((sk->sk_state != SMC_ACTIVE) &&
>> -	    (sk->sk_state != SMC_APPCLOSEWAIT1) &&
>> -	    (sk->sk_state != SMC_INIT))
>> -		goto out;
>>   
>> +	/* SMC do not support connect with fastopen */
>>   	if (msg->msg_flags & MSG_FASTOPEN) {
>> +		rc = -EINVAL;
>> +		lock_sock(sk);
>> +		/* not perform connect yet, fallback it */
>>   		if (sk->sk_state == SMC_INIT && !smc->connect_nonblock) {
>>   			rc = smc_switch_to_fallback(smc, SMC_CLC_DECL_OPTUNSUPP);
>> -			if (rc)
>> -				goto out;
>> -		} else {
>> -			rc = -EINVAL;
>> -			goto out;
>> +			/*  fallback success */
>> +			if (rc == 0)
>> +				goto fallback;	/* with sock lock hold */
>>   		}
>> +		release_sock(sk);
>> +		return rc;
>>   	}
>>   
>> +	lock_sock(sk);
>> +	if (sk->sk_state != SMC_ACTIVE &&
>> +	    sk->sk_state != SMC_APPCLOSEWAIT1 &&
>> +	    sk->sk_state != SMC_INIT)
>> +		goto out;
>> +
>>   	if (smc->use_fallback) {
>> +fallback:
>>   		rc = smc->clcsock->ops->sendmsg(smc->clcsock, msg, len);
>>   	} else {
>>   		rc = smc_tx_sendmsg(smc, msg, len);
>> -- 
>> 1.8.3.1
> Probably I messed something this, as this is *compile tested only*.
>
> But as the code at the out label looks like this:
>
> out:
>          release_sock(sk);
>          return rc;
>
> And smc_switch_to_fallback sets smc->use_fallback,
> I wonder if the following is a bit nicer:
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index a4cccdfdc00a..5d5c19e53b77 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -2657,16 +2657,14 @@ static int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>   {
>   	struct sock *sk = sock->sk;
>   	struct smc_sock *smc;
> -	int rc = -EPIPE;
> +	int rc;
>   
>   	smc = smc_sk(sk);
>   	lock_sock(sk);
> -	if ((sk->sk_state != SMC_ACTIVE) &&
> -	    (sk->sk_state != SMC_APPCLOSEWAIT1) &&
> -	    (sk->sk_state != SMC_INIT))
> -		goto out;
>   
> +	/* SMC does not support connect with fastopen */
>   	if (msg->msg_flags & MSG_FASTOPEN) {
> +		/* not connected yet, fallback */
>   		if (sk->sk_state == SMC_INIT && !smc->connect_nonblock) {
>   			rc = smc_switch_to_fallback(smc, SMC_CLC_DECL_OPTUNSUPP);
>   			if (rc)
> @@ -2675,6 +2673,11 @@ static int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>   			rc = -EINVAL;
>   			goto out;
>   		}
> +	} else if (sk->sk_state != SMC_ACTIVE &&
> +		   sk->sk_state != SMC_APPCLOSEWAIT1 &&
> +		   sk->sk_state != SMC_INIT) {
> +		rc = -EPIPE;
> +		goto out;
>   	}
>   
>   	if (smc->use_fallback) {
>