From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EC8D9C6FA99 for ; Mon, 6 Mar 2023 16:02:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229551AbjCFQBn (ORCPT ); Mon, 6 Mar 2023 11:01:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42436 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230160AbjCFQBM (ORCPT ); Mon, 6 Mar 2023 11:01:12 -0500 Received: from out30-132.freemail.mail.aliyun.com (out30-132.freemail.mail.aliyun.com [115.124.30.132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D7FBD34C39; Mon, 6 Mar 2023 08:01:04 -0800 (PST) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R191e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018045168;MF=alibuda@linux.alibaba.com;NM=1;PH=DS;RN=9;SR=0;TI=SMTPD_---0VdI0zlf_1678118461; Received: from 192.168.50.70(mailfrom:alibuda@linux.alibaba.com fp:SMTPD_---0VdI0zlf_1678118461) by smtp.aliyun-inc.com; Tue, 07 Mar 2023 00:01:02 +0800 Message-ID: <5e64b96e-5c8e-a631-287d-f960f52d8aaa@linux.alibaba.com> Date: Tue, 7 Mar 2023 00:01:01 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH net] net/smc: fix fallback failed while sendmsg with fastopen Content-Language: en-US To: Simon Horman Cc: kgraul@linux.ibm.com, wenjia@linux.ibm.com, jaka@linux.ibm.com, kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org, linux-s390@vger.kernel.org, linux-rdma@vger.kernel.org References: <1678075728-18812-1-git-send-email-alibuda@linux.alibaba.com> From: "D. Wythe" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-s390@vger.kernel.org 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" >> >> 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 >> --- >> 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) { >