diff mbox series

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

Message ID 1678075728-18812-1-git-send-email-alibuda@linux.alibaba.com (mailing list archive)
State Superseded
Headers show
Series [net] net/smc: fix fallback failed while sendmsg with fastopen | expand

Commit Message

D. Wythe March 6, 2023, 4:08 a.m. UTC 
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(-)

Comments

Simon Horman March 6, 2023, 12:24 p.m. UTC | #1 
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) {
D. Wythe March 6, 2023, 4:01 p.m. UTC | #2 
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) {
>
Simon Horman March 6, 2023, 4:02 p.m. UTC | #3 
On Tue, Mar 07, 2023 at 12:01:01AM +0800, D. Wythe wrote:
> 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?

Sure, thanks!

If you need it, my code can be:

Signed-off-by: Simon Horman <simon.horman@corigine.com>
D. Wythe March 6, 2023, 4:09 p.m. UTC | #4 
Roger that.

Thanks for you information.  I will issue the next version after the 
community

has no more comments.


On 3/7/23 12:02 AM, Simon Horman wrote:
> On Tue, Mar 07, 2023 at 12:01:01AM +0800, D. Wythe wrote:
>> 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?
> Sure, thanks!
>
> If you need it, my code can be:
>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
diff mbox series

Patch

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);