netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 0/3] bugfixs for smc
@ 2023-11-01  3:42 D. Wythe
  2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: D. Wythe @ 2023-11-01  3:42 UTC (permalink / raw)
  To: kgraul, wenjia, jaka, wintera; +Cc: kuba, davem, netdev, linux-s390, linux-rdma

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patches includes bugfix following:

1. hung state
2. sock leak
3. potential panic 

We have been testing these patches for some time, but
if you have any questions, please let us know.

Thanks,
D. Wythe

D. Wythe (3):
  net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT
  net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc
  net/smc: put sk reference if close work was canceled

 net/smc/af_smc.c    |  4 ++--
 net/smc/smc.h       |  5 +++++
 net/smc/smc_cdc.c   | 11 +++++------
 net/smc/smc_close.c |  5 +++--
 4 files changed, 15 insertions(+), 10 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT
  2023-11-01  3:42 [PATCH net 0/3] bugfixs for smc D. Wythe
@ 2023-11-01  3:42 ` D. Wythe
  2023-11-01  8:13   ` Dust Li
  2023-11-01  8:14   ` Dust Li
  2023-11-01  3:42 ` [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc D. Wythe
  2023-11-01  3:42 ` [PATCH net 3/3] net/smc: put sk reference if close work was canceled D. Wythe
  2 siblings, 2 replies; 8+ messages in thread
From: D. Wythe @ 2023-11-01  3:42 UTC (permalink / raw)
  To: kgraul, wenjia, jaka, wintera; +Cc: kuba, davem, netdev, linux-s390, linux-rdma

From: "D. Wythe" <alibuda@linux.alibaba.com>

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 <alibuda@linux.alibaba.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc
  2023-11-01  3:42 [PATCH net 0/3] bugfixs for smc D. Wythe
  2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
@ 2023-11-01  3:42 ` D. Wythe
  2023-11-01  8:19   ` Dust Li
  2023-11-01  3:42 ` [PATCH net 3/3] net/smc: put sk reference if close work was canceled D. Wythe
  2 siblings, 1 reply; 8+ messages in thread
From: D. Wythe @ 2023-11-01  3:42 UTC (permalink / raw)
  To: kgraul, wenjia, jaka, wintera; +Cc: kuba, davem, netdev, linux-s390, linux-rdma

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patch re-fix the issues memtianed by commit 22a825c541d7
("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()").

Blocking sending message do solve the issues though, but it also
prevents the peer to receive the final message. Besides, in logic,
whether the sndbuf_desc is NULL or not have no impact on the processing
of cdc message sending.

Hence that, this patch allow the cdc message sending but to check the
sndbuf_desc with care in smc_cdc_tx_handler().

Fixes: 22a825c541d7 ("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()")
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 net/smc/smc_cdc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 01bdb79..3c06625 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -28,13 +28,15 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd,
 {
 	struct smc_cdc_tx_pend *cdcpend = (struct smc_cdc_tx_pend *)pnd_snd;
 	struct smc_connection *conn = cdcpend->conn;
+	struct smc_buf_desc *sndbuf_desc;
 	struct smc_sock *smc;
 	int diff;
 
+	sndbuf_desc = conn->sndbuf_desc;
 	smc = container_of(conn, struct smc_sock, conn);
 	bh_lock_sock(&smc->sk);
-	if (!wc_status) {
-		diff = smc_curs_diff(cdcpend->conn->sndbuf_desc->len,
+	if (!wc_status && sndbuf_desc) {
+		diff = smc_curs_diff(sndbuf_desc->len,
 				     &cdcpend->conn->tx_curs_fin,
 				     &cdcpend->cursor);
 		/* sndbuf_space is decreased in smc_sendmsg */
@@ -114,9 +116,6 @@ int smc_cdc_msg_send(struct smc_connection *conn,
 	union smc_host_cursor cfed;
 	int rc;
 
-	if (unlikely(!READ_ONCE(conn->sndbuf_desc)))
-		return -ENOBUFS;
-
 	smc_cdc_add_pending_send(conn, pend);
 
 	conn->tx_cdc_seq++;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 3/3] net/smc: put sk reference if close work was canceled
  2023-11-01  3:42 [PATCH net 0/3] bugfixs for smc D. Wythe
  2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
  2023-11-01  3:42 ` [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc D. Wythe
@ 2023-11-01  3:42 ` D. Wythe
  2 siblings, 0 replies; 8+ messages in thread
From: D. Wythe @ 2023-11-01  3:42 UTC (permalink / raw)
  To: kgraul, wenjia, jaka, wintera; +Cc: kuba, davem, netdev, linux-s390, linux-rdma

From: "D. Wythe" <alibuda@linux.alibaba.com>

Note that we always hold a reference to sock when attempting
to submit close_work. Therefore, if we have successfully
canceled close_work from pending, we MUST release that reference
to avoid potential leaks.

Fixes: 42bfba9eaa33 ("net/smc: immediate termination for SMCD link groups")
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 net/smc/smc_close.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index 449ef45..10219f5 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -116,7 +116,8 @@ static void smc_close_cancel_work(struct smc_sock *smc)
 	struct sock *sk = &smc->sk;
 
 	release_sock(sk);
-	cancel_work_sync(&smc->conn.close_work);
+	if (cancel_work_sync(&smc->conn.close_work))
+		sock_put(sk);
 	cancel_delayed_work_sync(&smc->conn.tx_work);
 	lock_sock(sk);
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT
  2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
@ 2023-11-01  8:13   ` Dust Li
  2023-11-01  8:14   ` Dust Li
  1 sibling, 0 replies; 8+ messages in thread
From: Dust Li @ 2023-11-01  8:13 UTC (permalink / raw)
  To: D. Wythe, kgraul, wenjia, jaka, wintera
  Cc: kuba, davem, netdev, linux-s390, linux-rdma

On Wed, Nov 01, 2023 at 11:42:55AM +0800, D. Wythe wrote:
>From: "D. Wythe" <alibuda@linux.alibaba.com>
>
>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 <alibuda@linux.alibaba.com>
>Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Reviewed-by: Dust Li <dust.li@linux.alibaba.com>

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT
  2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
  2023-11-01  8:13   ` Dust Li
@ 2023-11-01  8:14   ` Dust Li
  1 sibling, 0 replies; 8+ messages in thread
From: Dust Li @ 2023-11-01  8:14 UTC (permalink / raw)
  To: D. Wythe, kgraul, wenjia, jaka, wintera
  Cc: kuba, davem, netdev, linux-s390, linux-rdma

On Wed, Nov 01, 2023 at 11:42:55AM +0800, D. Wythe wrote:
>From: "D. Wythe" <alibuda@linux.alibaba.com>
>
>Considering scenario:
>
>				smc_cdc_rx_handler_rwwi

Nit, smc_cdc_rx_handler_rwwi should be smc_cdc_rx_handler()

>__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 <alibuda@linux.alibaba.com>
>Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
>---
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc
  2023-11-01  3:42 ` [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc D. Wythe
@ 2023-11-01  8:19   ` Dust Li
  2023-11-01  8:36     ` D. Wythe
  0 siblings, 1 reply; 8+ messages in thread
From: Dust Li @ 2023-11-01  8:19 UTC (permalink / raw)
  To: D. Wythe, kgraul, wenjia, jaka, wintera
  Cc: kuba, davem, netdev, linux-s390, linux-rdma

On Wed, Nov 01, 2023 at 11:42:56AM +0800, D. Wythe wrote:
>From: "D. Wythe" <alibuda@linux.alibaba.com>
>
>This patch re-fix the issues memtianed by commit 22a825c541d7

memtianed -> mentioned ?

>("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()").
>
>Blocking sending message do solve the issues though, but it also
>prevents the peer to receive the final message. Besides, in logic,
>whether the sndbuf_desc is NULL or not have no impact on the processing
>of cdc message sending.
>
>Hence that, this patch allow the cdc message sending but to check the
allows

>sndbuf_desc with care in smc_cdc_tx_handler().
>
>Fixes: 22a825c541d7 ("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()")
>Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Reviewed-by: Dust Li <dust.li@linux.alibaba.com>

>---
> net/smc/smc_cdc.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
>diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
>index 01bdb79..3c06625 100644
>--- a/net/smc/smc_cdc.c
>+++ b/net/smc/smc_cdc.c
>@@ -28,13 +28,15 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd,
> {
> 	struct smc_cdc_tx_pend *cdcpend = (struct smc_cdc_tx_pend *)pnd_snd;
> 	struct smc_connection *conn = cdcpend->conn;
>+	struct smc_buf_desc *sndbuf_desc;
> 	struct smc_sock *smc;
> 	int diff;
> 
>+	sndbuf_desc = conn->sndbuf_desc;
> 	smc = container_of(conn, struct smc_sock, conn);
> 	bh_lock_sock(&smc->sk);
>-	if (!wc_status) {
>-		diff = smc_curs_diff(cdcpend->conn->sndbuf_desc->len,
>+	if (!wc_status && sndbuf_desc) {
>+		diff = smc_curs_diff(sndbuf_desc->len,
> 				     &cdcpend->conn->tx_curs_fin,
> 				     &cdcpend->cursor);
> 		/* sndbuf_space is decreased in smc_sendmsg */
>@@ -114,9 +116,6 @@ int smc_cdc_msg_send(struct smc_connection *conn,
> 	union smc_host_cursor cfed;
> 	int rc;
> 
>-	if (unlikely(!READ_ONCE(conn->sndbuf_desc)))
>-		return -ENOBUFS;
>-
> 	smc_cdc_add_pending_send(conn, pend);
> 
> 	conn->tx_cdc_seq++;
>-- 
>1.8.3.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc
  2023-11-01  8:19   ` Dust Li
@ 2023-11-01  8:36     ` D. Wythe
  0 siblings, 0 replies; 8+ messages in thread
From: D. Wythe @ 2023-11-01  8:36 UTC (permalink / raw)
  To: dust.li, kgraul, wenjia, jaka, wintera
  Cc: kuba, davem, netdev, linux-s390, linux-rdma




On 11/1/23 4:19 PM, Dust Li wrote:
> On Wed, Nov 01, 2023 at 11:42:56AM +0800, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> This patch re-fix the issues memtianed by commit 22a825c541d7
> memtianed -> mentioned ?
>
>> ("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()").
>>
>> Blocking sending message do solve the issues though, but it also
>> prevents the peer to receive the final message. Besides, in logic,
>> whether the sndbuf_desc is NULL or not have no impact on the processing
>> of cdc message sending.
>>
>> Hence that, this patch allow the cdc message sending but to check the
> allows
>
>> sndbuf_desc with care in smc_cdc_tx_handler().
>>
>> Fixes: 22a825c541d7 ("net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler()")
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Thanks for that. I will fix them in next version.

> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
>> ---
>> net/smc/smc_cdc.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
>> index 01bdb79..3c06625 100644
>> --- a/net/smc/smc_cdc.c
>> +++ b/net/smc/smc_cdc.c
>> @@ -28,13 +28,15 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd,
>> {
>> 	struct smc_cdc_tx_pend *cdcpend = (struct smc_cdc_tx_pend *)pnd_snd;
>> 	struct smc_connection *conn = cdcpend->conn;
>> +	struct smc_buf_desc *sndbuf_desc;
>> 	struct smc_sock *smc;
>> 	int diff;
>>
>> +	sndbuf_desc = conn->sndbuf_desc;
>> 	smc = container_of(conn, struct smc_sock, conn);
>> 	bh_lock_sock(&smc->sk);
>> -	if (!wc_status) {
>> -		diff = smc_curs_diff(cdcpend->conn->sndbuf_desc->len,
>> +	if (!wc_status && sndbuf_desc) {
>> +		diff = smc_curs_diff(sndbuf_desc->len,
>> 				     &cdcpend->conn->tx_curs_fin,
>> 				     &cdcpend->cursor);
>> 		/* sndbuf_space is decreased in smc_sendmsg */
>> @@ -114,9 +116,6 @@ int smc_cdc_msg_send(struct smc_connection *conn,
>> 	union smc_host_cursor cfed;
>> 	int rc;
>>
>> -	if (unlikely(!READ_ONCE(conn->sndbuf_desc)))
>> -		return -ENOBUFS;
>> -
>> 	smc_cdc_add_pending_send(conn, pend);
>>
>> 	conn->tx_cdc_seq++;
>> -- 
>> 1.8.3.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-11-01  8:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-01  3:42 [PATCH net 0/3] bugfixs for smc D. Wythe
2023-11-01  3:42 ` [PATCH net 1/3] net/smc: fix dangling sock under state SMC_APPFINCLOSEWAIT D. Wythe
2023-11-01  8:13   ` Dust Li
2023-11-01  8:14   ` Dust Li
2023-11-01  3:42 ` [PATCH net 2/3] net/smc: allow cdc msg send rather than drop it with NULL sndbuf_desc D. Wythe
2023-11-01  8:19   ` Dust Li
2023-11-01  8:36     ` D. Wythe
2023-11-01  3:42 ` [PATCH net 3/3] net/smc: put sk reference if close work was canceled D. Wythe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).