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

"D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx> · Tue, 7 Mar 2023 00:01:01 +0800

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@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) {







