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

Wenjia Zhang
2024-11-29T17:00:55Z

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