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

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

[Hidayath Khan]

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: stable@vger.kernel.org
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 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

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

[Dust Li]

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: stable@vger.kernel.org
>Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
>Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>

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

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

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "*shiko finds more issues with the work termination here.
Can you please TAL and fix these all in one series?"
---
net/smc: drop the abort_work reference when the work is cancelled

The patch makes smc_conn_free() drop the socket reference handed to
conn->abort_work when cancel_work_sync() reports that the work was still
pending, so the sock_hold() taken by the scheduler of the work is not
leaked.  It follows the pattern smc_close_cancel_work() already uses for
conn->close_work.

A few questions about the surrounding cancel logic follow.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5e..3d4a86d99dd6b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1252,6 +1252,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)
> @@ -1275,8 +1276,13 @@ static void smc_buf_unuse(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);
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but can this cancel_work_sync()
deadlock against a already-running abort_work?

The worker takes the socket lock as its first action:

net/smc/smc_core.c:smc_conn_abort_work() {
	lock_sock(&smc->sk);
	smc_conn_kill(conn, true);
	release_sock(&smc->sk);
	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
}

and the callers of smc_conn_free() reach it while holding that same lock:

net/smc/af_smc.c:__smc_release()
	lock_sock(sk);
	...
	if (!smc->use_fallback)
		smc_conn_free(&smc->conn);

net/smc/smc_close.c:smc_close_passive_work()  (inside lock_sock(sk))
	if ((sk->sk_state == SMC_CLOSED) &&
	    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
		smc_conn_free(conn);

net/smc/smc_close.c:smc_close_active_abort()  (SMC_PEERCLOSEWAIT1/2,
SMC_PEERFINCLOSEWAIT, SMC_PROCESSABORT, SMC_APPFINCLOSEWAIT)
		smc_conn_free(&smc->conn);

So if abort_work has already started on another CPU and is blocked in
lock_sock(), the closing task holds the socket lock and waits in
cancel_work_sync() for that worker, which cannot proceed.  The
current_work() != &conn->abort_work test only covers the case where the
same task is running abort_work itself.

Note both abort_work and close_work are queued on smc_close_wq, so
close_work running smc_close_passive_work() can be the blocking canceller.

The commit message cites smc_close_cancel_work() as the model:

	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);

but that helper drops the socket lock around the cancel:

net/smc/smc_close.c:smc_close_cancel_work() {
	release_sock(sk);
	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);
	cancel_delayed_work_sync(&smc->conn.tx_work);
	lock_sock(sk);
}

Should the abort_work cancel do the same, and would that change where the
new sock_put() has to live?

[Severity: High]
This is a pre-existing ordering issue rather than something the patch adds,
but does cancel_work_sync() here actually keep abort_work from being
re-armed?

The cancel happens before smc_lgr_unregister_conn(conn), so the receive
path can still find the connection:

net/smc/smc_cdc.c:smc_cdc_rx_handler() {
	read_lock_bh(&lgr->conns_lock);
	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
	if (!conn || conn->out_of_sync) {
		read_unlock_bh(&lgr->conns_lock);
		return;
	}
	smc = container_of(conn, struct smc_sock, conn);
	sock_hold(&smc->sk);
	read_unlock_bh(&lgr->conns_lock);

and then queue the work again:

net/smc/smc_cdc.c:smc_cdc_msg_validate() {
		sock_hold(&smc->sk); /* sock_put in abort_work */
		if (!queue_work(smc_close_wq, &conn->abort_work))
			sock_put(&smc->sk);

cancel_work_sync() passes cflags 0, so it re-enables the work before
returning:

kernel/workqueue.c:__cancel_work_sync() {
	ret = __cancel_work(work, cflags | WORK_CANCEL_DISABLE);
	...
	if (wq_online)
		__flush_work(work, true);

	if (!(cflags & WORK_CANCEL_DISABLE))
		enable_work(work);

which means a queue_work() issued after the cancel returns succeeds.  The
freshly queued worker then runs smc_conn_abort_work() -> smc_conn_kill() on
a connection with conn->freed == 1 whose lgr reference smc_conn_free()
already dropped, and dereferences conn->lgr:

net/smc/smc_core.c:smc_conn_kill() {
	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)

Would unregistering the connection from lgr->conns_all before cancelling,
or gating the queueing on conn->freed under conns_lock, close that window?

>  	}
>  	if (!list_empty(&lgr->list)) {
>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */

[Severity: High]
This is also a pre-existing problem and not something introduced here, but
the patch documents smc_conn_free() as the place that disposes of a pending
abort_work, and one exit path of the function skips that disposal:

net/smc/smc_core.c:smc_conn_free() {
	if (!smc_conn_lgr_valid(conn))
		/* Connection has already unregistered from
		 * link group.
		 */
		goto lgr_put;
	...
lgr_put:
	if (!lgr->is_smcd)
		smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

On that path neither the cancel nor the new sock_put() runs, yet the
connection's link and link-group references are still dropped, and
conn->lgr is not reset.  A pending abort_work then dereferences it:

net/smc/smc_core.c:smc_conn_kill() {
	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)

Concretely, a peer sends a CDC message with prod_flags.failover_validation
and a stale seqno, so smc_cdc_msg_validate() queues abort_work.  Meanwhile
__smc_lgr_terminate() calls smc_conn_kill(), which does
smc_lgr_unregister_conn() (clearing alert_token_local) and then
smc_close_active_abort() -> smc_conn_free(); smc_conn_lgr_valid(conn) is
now false, so the goto lgr_put path is taken.  __smc_lgr_terminate() then
calls smc_lgr_free(), whose smc_lgr_put() drops the last reference and
kfree()s the lgr while abort_work is still queued.

Can the queued abort_work read a freed struct smc_link_group here, and
should this exit path also cancel the work and return its reference?