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

[Simon Horman <simon.horman@xxxxxxxxxxxx>]

On Mon, Mar 06, 2023 at 12:08:48PM +0800, D. Wythe wrote:
> From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxx>
> ---
>  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) {