From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3B35063AB for ; Wed, 1 Nov 2023 08:13:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: from out30-113.freemail.mail.aliyun.com (out30-113.freemail.mail.aliyun.com [115.124.30.113]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 98963B4; Wed, 1 Nov 2023 01:13:32 -0700 (PDT) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R311e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046049;MF=dust.li@linux.alibaba.com;NM=1;PH=DS;RN=10;SR=0;TI=SMTPD_---0VvQMaox_1698826408; Received: from localhost(mailfrom:dust.li@linux.alibaba.com fp:SMTPD_---0VvQMaox_1698826408) by smtp.aliyun-inc.com; Wed, 01 Nov 2023 16:13:29 +0800 Date: Wed, 1 Nov 2023 16:13:28 +0800 From: Dust Li To: "D. Wythe" , kgraul@linux.ibm.com, wenjia@linux.ibm.com, jaka@linux.ibm.com, wintera@linux.ibm.com Cc: kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org, linux-s390@vger.kernel.org, linux-rdma@vger.kernel.org Subject: Re: [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT Message-ID: <20231101081328.GE92403@linux.alibaba.com> Reply-To: dust.li@linux.alibaba.com References: <1698810177-69740-1-git-send-email-alibuda@linux.alibaba.com> <1698810177-69740-2-git-send-email-alibuda@linux.alibaba.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1698810177-69740-2-git-send-email-alibuda@linux.alibaba.com> On Wed, Nov 01, 2023 at 11:42:55AM +0800, D. Wythe wrote: >From: "D. Wythe" > >Considering scenario: > > smc_cdc_rx_handler_rwwi >__smc_release > sock_set_flag >smc_close_active() >sock_set_flag > >__set_bit(DEAD) __set_bit(DONE) > >Dues to __set_bit is not atomic, the DEAD or DONE might be lost. >if the DEAD flag lost, the state SMC_CLOSED will be never be reached >in smc_close_passive_work: > >if (sock_flag(sk, SOCK_DEAD) && > smc_close_sent_any_close(conn)) { > sk->sk_state = SMC_CLOSED; >} else { > /* just shutdown, but not yet closed locally */ > sk->sk_state = SMC_APPFINCLOSEWAIT; >} > >Replace sock_set_flags or __set_bit to set_bit will fix this problem. >Since set_bit is atomic. > >Signed-off-by: D. Wythe >Reviewed-by: Wenjia Zhang Reviewed-by: Dust Li >--- > net/smc/af_smc.c | 4 ++-- > net/smc/smc.h | 5 +++++ > net/smc/smc_cdc.c | 2 +- > net/smc/smc_close.c | 2 +- > 4 files changed, 9 insertions(+), 4 deletions(-) > >diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c >index abd2667..da97f94 100644 >--- a/net/smc/af_smc.c >+++ b/net/smc/af_smc.c >@@ -275,7 +275,7 @@ static int __smc_release(struct smc_sock *smc) > > if (!smc->use_fallback) { > rc = smc_close_active(smc); >- sock_set_flag(sk, SOCK_DEAD); >+ smc_sock_set_flag(sk, SOCK_DEAD); > sk->sk_shutdown |= SHUTDOWN_MASK; > } else { > if (sk->sk_state != SMC_CLOSED) { >@@ -1743,7 +1743,7 @@ static int smc_clcsock_accept(struct smc_sock *lsmc, struct smc_sock **new_smc) > if (new_clcsock) > sock_release(new_clcsock); > new_sk->sk_state = SMC_CLOSED; >- sock_set_flag(new_sk, SOCK_DEAD); >+ smc_sock_set_flag(new_sk, SOCK_DEAD); > sock_put(new_sk); /* final */ > *new_smc = NULL; > goto out; >diff --git a/net/smc/smc.h b/net/smc/smc.h >index 24745fd..e377980 100644 >--- a/net/smc/smc.h >+++ b/net/smc/smc.h >@@ -377,4 +377,9 @@ void smc_fill_gid_list(struct smc_link_group *lgr, > int smc_nl_enable_hs_limitation(struct sk_buff *skb, struct genl_info *info); > int smc_nl_disable_hs_limitation(struct sk_buff *skb, struct genl_info *info); > >+static inline void smc_sock_set_flag(struct sock *sk, enum sock_flags flag) >+{ >+ set_bit(flag, &sk->sk_flags); >+} >+ > #endif /* __SMC_H */ >diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c >index 89105e9..01bdb79 100644 >--- a/net/smc/smc_cdc.c >+++ b/net/smc/smc_cdc.c >@@ -385,7 +385,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc, > smc->sk.sk_shutdown |= RCV_SHUTDOWN; > if (smc->clcsock && smc->clcsock->sk) > smc->clcsock->sk->sk_shutdown |= RCV_SHUTDOWN; >- sock_set_flag(&smc->sk, SOCK_DONE); >+ smc_sock_set_flag(&smc->sk, SOCK_DONE); > sock_hold(&smc->sk); /* sock_put in close_work */ > if (!queue_work(smc_close_wq, &conn->close_work)) > sock_put(&smc->sk); >diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c >index dbdf03e..449ef45 100644 >--- a/net/smc/smc_close.c >+++ b/net/smc/smc_close.c >@@ -173,7 +173,7 @@ void smc_close_active_abort(struct smc_sock *smc) > break; > } > >- sock_set_flag(sk, SOCK_DEAD); >+ smc_sock_set_flag(sk, SOCK_DEAD); > sk->sk_state_change(sk); > > if (release_clcsock) { >-- >1.8.3.1