Re: [PATCH net] net/smc: drop the abort_work reference when the work is cancelled

On 2026-08-06 10:15:49, Hidayath Khan wrote:
>The schedulers of conn->abort_work hand a socket reference to the work
>item and rely on it to give the reference back:
>
>        sock_hold(&smc->sk); /* sock_put in abort_work */
>        if (!queue_work(smc_close_wq, &conn->abort_work))
>                sock_put(&smc->sk);
>
>The queue_work() failure case is handled, but the cancellation case is
>not. smc_conn_free() cancels a still-pending abort_work:
>
>        if (current_work() != &conn->abort_work)
>                cancel_work_sync(&conn->abort_work);
>
>and discards the return value.  When cancel_work_sync() returns true the
>work was queued but had not started, so smc_conn_abort_work() never runs
>and its sock_put() never happens.  The reference is lost.
>
>As in smc_switch_conns(), a leaked sk_refcnt means the smc_sock is never
>destroyed: its buffers stay allocated and the network namespace reference
>a user socket holds is never released, so the netns cannot be torn down.
>
>smc_cdc_msg_validate() queues abort_work from the receive tasklet when a
>peer sends a CDC message with an out-of-order sequence number, so a
>remote peer combined with a concurrent local close is enough to reach it.
>
>Drop the reference when the work is cancelled, matching the pattern
>smc_close_cancel_work() already uses for conn->close_work:
>
>        if (cancel_work_sync(&smc->conn.close_work))
>                sock_put(sk);
>
>The sock_put() is safe here: every caller of smc_conn_free() passes the
>connection of a socket it holds a reference to, so this cannot release
>the last one.
>
>Fixes: b286a0651e44 ("net/smc: handle incoming CDC validation message")
>Cc: [email protected]
>Reviewed-by: Mahanta Jambigi <[email protected]>
>Reviewed-by: Sidraya Jayagond <[email protected]>
>Signed-off-by: Hidayath Khan <[email protected]>

Reviewed-by: Dust Li <[email protected]>

Best regards,
Dust

>---
> net/smc/smc_core.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>index c0027d2fe4e8..dd9fffbe41e0 100644
>--- a/net/smc/smc_core.c
>+++ b/net/smc/smc_core.c
>@@ -1254,6 +1254,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
> /* remove a finished connection from its link group */
> void smc_conn_free(struct smc_connection *conn)
> {
>+	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
> 	struct smc_link_group *lgr = conn->lgr;
> 
> 	if (!lgr || conn->freed)
>@@ -1277,8 +1278,13 @@ void smc_conn_free(struct smc_connection *conn)
> 		tasklet_kill(&conn->rx_tsklet);
> 	} else {
> 		smc_cdc_wait_pend_tx_wr(conn);
>-		if (current_work() != &conn->abort_work)
>-			cancel_work_sync(&conn->abort_work);
>+		/* If the work was pending (cancel returns true) it never ran,
>+		 * so the sock_hold taken by its scheduler was never released.
>+		 */
>+		if (current_work() != &conn->abort_work) {
>+			if (cancel_work_sync(&conn->abort_work))
>+				sock_put(&smc->sk);
>+		}
> 	}
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
>
>base-commit: 2b1c2bc2355fd59cc75045e42d9dc5470ef5fa9a
>-- 
>2.52.0