[PATCH net v2 0/2] two fixes for SMC

[PATCH net v2 0/2] two fixes for SMC

[Wen Gu]

Hi, all

This patch set contains two bugfixes, to fix SMC warning and panic
issues in race conditions.

Thanks!

v2->v1:
(https://lore.kernel.org/netdev/20241122071630.63707-2-guwen@linux.alibaba.com/)
- Patch #1: Collect 'Reviewed-by' tags;
- Patch #2: Extend sock lock protection from smc_conn_free() in smc_conn_abort() to most of smc_listen_work().

Wen Gu (2):
  net/smc: initialize close_work early to avoid warning
  net/smc: fix LGR and link use-after-free issue

 net/smc/af_smc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH net v2 1/2] net/smc: initialize close_work early to avoid warning

[Wen Gu]

We encountered a warning that close_work was canceled before
initialization.

  WARNING: CPU: 7 PID: 111103 at kernel/workqueue.c:3047 __flush_work+0x19e/0x1b0
  Workqueue: events smc_lgr_terminate_work [smc]
  RIP: 0010:__flush_work+0x19e/0x1b0
  Call Trace:
   ? __wake_up_common+0x7a/0x190
   ? work_busy+0x80/0x80
   __cancel_work_timer+0xe3/0x160
   smc_close_cancel_work+0x1a/0x70 [smc]
   smc_close_active_abort+0x207/0x360 [smc]
   __smc_lgr_terminate.part.38+0xc8/0x180 [smc]
   process_one_work+0x19e/0x340
   worker_thread+0x30/0x370
   ? process_one_work+0x340/0x340
   kthread+0x117/0x130
   ? __kthread_cancel_work+0x50/0x50
   ret_from_fork+0x22/0x30

This is because when smc_close_cancel_work is triggered, e.g. the RDMA
driver is rmmod and the LGR is terminated, the conn->close_work is
flushed before initialization, resulting in WARN_ON(!work->func).

__smc_lgr_terminate             | smc_connect_{rdma|ism}
-------------------------------------------------------------
                                | smc_conn_create
				| \- smc_lgr_register_conn
for conn in lgr->conns_all      |
\- smc_conn_kill                |
   \- smc_close_active_abort    |
      \- smc_close_cancel_work  |
         \- cancel_work_sync    |
            \- __flush_work     |
	         (close_work)   |
	                        | smc_close_init
	                        | \- INIT_WORK(&close_work)

So fix this by initializing close_work before establishing the
connection.

Fixes: 46c28dbd4c23 ("net/smc: no socket state changes in tasklet context")
Fixes: 413498440e30 ("net/smc: add SMC-D support in af_smc")
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
---
 net/smc/af_smc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 9d76e902fd77..ed6d4d520bc7 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -383,6 +383,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
 	smc->limit_smc_hs = net->smc.limit_smc_hs;
 	smc->use_fallback = false; /* assume rdma capability first */
 	smc->fallback_rsn = 0;
+	smc_close_init(smc);
 }
 
 static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
@@ -1299,7 +1300,6 @@ static int smc_connect_rdma(struct smc_sock *smc,
 		goto connect_abort;
 	}
 
-	smc_close_init(smc);
 	smc_rx_init(smc);
 
 	if (ini->first_contact_local) {
@@ -1435,7 +1435,6 @@ static int smc_connect_ism(struct smc_sock *smc,
 			goto connect_abort;
 		}
 	}
-	smc_close_init(smc);
 	smc_rx_init(smc);
 	smc_tx_init(smc);
 
@@ -2479,7 +2478,6 @@ static void smc_listen_work(struct work_struct *work)
 		goto out_decl;
 
 	mutex_lock(&smc_server_lgr_pending);
-	smc_close_init(new_smc);
 	smc_rx_init(new_smc);
 	smc_tx_init(new_smc);
 
-- 
2.32.0.3.g01195cf9f

[PATCH net v2 2/2] net/smc: fix LGR and link use-after-free issue

[Wen Gu]

We encountered a LGR/link use-after-free issue, which manifested as
the LGR/link refcnt reaching 0 early and entering the clear process,
making resource access unsafe.

 refcount_t: addition on 0; use-after-free.
 WARNING: CPU: 14 PID: 107447 at lib/refcount.c:25 refcount_warn_saturate+0x9c/0x140
 Workqueue: events smc_lgr_terminate_work [smc]
 Call trace:
  refcount_warn_saturate+0x9c/0x140
  __smc_lgr_terminate.part.45+0x2a8/0x370 [smc]
  smc_lgr_terminate_work+0x28/0x30 [smc]
  process_one_work+0x1b8/0x420
  worker_thread+0x158/0x510
  kthread+0x114/0x118

or

 refcount_t: underflow; use-after-free.
 WARNING: CPU: 6 PID: 93140 at lib/refcount.c:28 refcount_warn_saturate+0xf0/0x140
 Workqueue: smc_hs_wq smc_listen_work [smc]
 Call trace:
  refcount_warn_saturate+0xf0/0x140
  smcr_link_put+0x1cc/0x1d8 [smc]
  smc_conn_free+0x110/0x1b0 [smc]
  smc_conn_abort+0x50/0x60 [smc]
  smc_listen_find_device+0x75c/0x790 [smc]
  smc_listen_work+0x368/0x8a0 [smc]
  process_one_work+0x1b8/0x420
  worker_thread+0x158/0x510
  kthread+0x114/0x118

It is caused by repeated release of LGR/link refcnt. One suspect is that
smc_conn_free() is called repeatedly because some smc_conn_free() from
server listening path are not protected by sock lock.

e.g.

Calls under socklock        | smc_listen_work
-------------------------------------------------------
lock_sock(sk)               | smc_conn_abort
smc_conn_free               | \- smc_conn_free
\- smcr_link_put            |    \- smcr_link_put (duplicated)
release_sock(sk)

So here add sock lock protection in smc_listen_work() path, making it
exclusive with other connection operations.

Fixes: 3b2dec2603d5 ("net/smc: restructure client and server code in af_smc")
Co-developed-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Co-developed-by: Kai <KaiShen@linux.alibaba.com>
Signed-off-by: Kai <KaiShen@linux.alibaba.com>
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/af_smc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index ed6d4d520bc7..9e6c69d18581 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1900,6 +1900,7 @@ static void smc_listen_out(struct smc_sock *new_smc)
 	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
 		atomic_dec(&lsmc->queued_smc_hs);
 
+	release_sock(newsmcsk); /* lock in smc_listen_work() */
 	if (lsmc->sk.sk_state == SMC_LISTEN) {
 		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
 		smc_accept_enqueue(&lsmc->sk, newsmcsk);
@@ -2421,6 +2422,7 @@ static void smc_listen_work(struct work_struct *work)
 	u8 accept_version;
 	int rc = 0;
 
+	lock_sock(&new_smc->sk); /* release in smc_listen_out() */
 	if (new_smc->listen_smc->sk.sk_state != SMC_LISTEN)
 		return smc_listen_out_err(new_smc);
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net v2 2/2] net/smc: fix LGR and link use-after-free issue

[Alexandra Winter]

On 27.11.24 14:30, Wen Gu wrote:
>  net/smc/af_smc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index ed6d4d520bc7..9e6c69d18581 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1900,6 +1900,7 @@ static void smc_listen_out(struct smc_sock *new_smc)
>  	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
>  		atomic_dec(&lsmc->queued_smc_hs);
>  
> +	release_sock(newsmcsk); /* lock in smc_listen_work() */
>  	if (lsmc->sk.sk_state == SMC_LISTEN) {
>  		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
>  		smc_accept_enqueue(&lsmc->sk, newsmcsk);
> @@ -2421,6 +2422,7 @@ static void smc_listen_work(struct work_struct *work)
>  	u8 accept_version;
>  	int rc = 0;
>  
> +	lock_sock(&new_smc->sk); /* release in smc_listen_out() */
>  	if (new_smc->listen_smc->sk.sk_state != SMC_LISTEN)
>  		return smc_listen_out_err(new_smc);
>  

As far as I can tell, this looks good to me.
Unfortunately I don't understand the dependencies between the different SMC sockets and TCP sockets
well enough to give an R-b.

Re: [PATCH net v2 2/2] net/smc: fix LGR and link use-after-free issue

[Wenjia Zhang]

On 27.11.24 14:30, Wen Gu wrote:
> We encountered a LGR/link use-after-free issue, which manifested as
> the LGR/link refcnt reaching 0 early and entering the clear process,
> making resource access unsafe.
> 
>   refcount_t: addition on 0; use-after-free.
>   WARNING: CPU: 14 PID: 107447 at lib/refcount.c:25 refcount_warn_saturate+0x9c/0x140
>   Workqueue: events smc_lgr_terminate_work [smc]
>   Call trace:
>    refcount_warn_saturate+0x9c/0x140
>    __smc_lgr_terminate.part.45+0x2a8/0x370 [smc]
>    smc_lgr_terminate_work+0x28/0x30 [smc]
>    process_one_work+0x1b8/0x420
>    worker_thread+0x158/0x510
>    kthread+0x114/0x118
> 
> or
> 
>   refcount_t: underflow; use-after-free.
>   WARNING: CPU: 6 PID: 93140 at lib/refcount.c:28 refcount_warn_saturate+0xf0/0x140
>   Workqueue: smc_hs_wq smc_listen_work [smc]
>   Call trace:
>    refcount_warn_saturate+0xf0/0x140
>    smcr_link_put+0x1cc/0x1d8 [smc]
>    smc_conn_free+0x110/0x1b0 [smc]
>    smc_conn_abort+0x50/0x60 [smc]
>    smc_listen_find_device+0x75c/0x790 [smc]
>    smc_listen_work+0x368/0x8a0 [smc]
>    process_one_work+0x1b8/0x420
>    worker_thread+0x158/0x510
>    kthread+0x114/0x118
> 
> It is caused by repeated release of LGR/link refcnt. One suspect is that
> smc_conn_free() is called repeatedly because some smc_conn_free() from
> server listening path are not protected by sock lock.
> 
> e.g.
> 
> Calls under socklock        | smc_listen_work
> -------------------------------------------------------
> lock_sock(sk)               | smc_conn_abort
> smc_conn_free               | \- smc_conn_free
> \- smcr_link_put            |    \- smcr_link_put (duplicated)
> release_sock(sk)
> 
> So here add sock lock protection in smc_listen_work() path, making it
> exclusive with other connection operations.
> 
> Fixes: 3b2dec2603d5 ("net/smc: restructure client and server code in af_smc")
> Co-developed-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Co-developed-by: Kai <KaiShen@linux.alibaba.com>
> Signed-off-by: Kai <KaiShen@linux.alibaba.com>
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
>   net/smc/af_smc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index ed6d4d520bc7..9e6c69d18581 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1900,6 +1900,7 @@ static void smc_listen_out(struct smc_sock *new_smc)
>   	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
>   		atomic_dec(&lsmc->queued_smc_hs);
>   
> +	release_sock(newsmcsk); /* lock in smc_listen_work() */
>   	if (lsmc->sk.sk_state == SMC_LISTEN) {
>   		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
>   		smc_accept_enqueue(&lsmc->sk, newsmcsk);
> @@ -2421,6 +2422,7 @@ static void smc_listen_work(struct work_struct *work)
>   	u8 accept_version;
>   	int rc = 0;
>   
> +	lock_sock(&new_smc->sk); /* release in smc_listen_out() */
>   	if (new_smc->listen_smc->sk.sk_state != SMC_LISTEN)
>   		return smc_listen_out_err(new_smc);
>   

It looked much clearer than the last version to me! Thank you for fixing it!

Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Thanks,
Wenjia

Re: [PATCH net v2 0/2] two fixes for SMC

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 27 Nov 2024 21:30:12 +0800 you wrote:
> Hi, all
> 
> This patch set contains two bugfixes, to fix SMC warning and panic
> issues in race conditions.
> 
> Thanks!
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] net/smc: initialize close_work early to avoid warning
    https://git.kernel.org/netdev/net/c/0541db8ee32c
  - [net,v2,2/2] net/smc: fix LGR and link use-after-free issue
    https://git.kernel.org/netdev/net/c/2c7f14ed9c19

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html