[PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()

[PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()

[Hidayath Khan]

smc_conn_free() disposes of a pending conn->abort_work, but it gets three
things wrong:

1. Deadlock: smc_conn_free() runs with the socket lock held and calls
   cancel_work_sync(), while smc_conn_abort_work() takes the same lock.
   If the work has already started on another CPU and is waiting for that
   lock, the cancel waits for the work and the work waits for the caller.
   The current_work() test only stops the work from cancelling itself,
   not when the two run on different CPUs.

2. Reference leak: Schedulers of abort_work take a socket reference, and
   smc_conn_abort_work() drops it when it runs. If cancel_work_sync()
   removes a pending work item before it runs, that reference is never
   returned and the socket is never freed.

Both are fixed the way smc_close_cancel_work() handles close_work: drop
the socket lock around the cancel, and release the reference when the
cancel reports that it removed a pending item.

3. Late-queued work race: smc_cdc_rx_handler() finds the connection and
   drops lgr->conns_lock before smc_cdc_msg_validate() decides to queue:

     CPU0 (smc_conn_free)              CPU1 (smc_cdc_rx_handler)
                                       conn = smc_lgr_find_conn()
                                       sock_hold()
                                       read_unlock_bh(&lgr->conns_lock)
     cancel_work_sync()                  /* nothing queued yet */
     smc_buf_unuse()
                                       smc_cdc_msg_validate()
                                         queue_work(&conn->abort_work)

   cancel_work_sync() only guarantees that the work is not pending or
   running when it returns; a racing enqueue lands after that. The work
   then calls smc_conn_kill() on a connection whose buffers have already
   been returned.

Nothing smc_conn_free() does can prevent that enqueue, because the
receiver already holds the connection pointer. Make the late work
harmless instead: smc_conn_free() sets conn->freed with the socket lock
held before it releases anything, and smc_conn_abort_work() takes the same
lock. Check conn->freed inside smc_conn_abort_work() to skip
smc_conn_kill() if teardown has started. The work still drops its socket
reference.

Fixes: b286a0651e44 ("net/smc: handle incoming CDC validation message")
Cc: stable@vger.kernel.org
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
v2:
- Extended the fix to cover the deadlock and racing enqueue issues flagged
  during v1 review.
- Moved the cancel into a helper smc_conn_cancel_abort_work() that drops
  the socket lock around cancel_work_sync().
- Added a check for conn->freed under lock_sock in smc_conn_abort_work() to
  safely handle late-queued work items without fragile reordering.
- Updated patch subject to reflect the broader termination fix.
  Link: https://lore.kernel.org/netdev/20260806081549.595001-1-hidayath@linux.ibm.com/

 net/smc/smc_core.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 04aedd957543..9a109eae73b9 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1251,6 +1251,25 @@ static void smc_buf_unuse(struct smc_connection *conn,
 	}
 }
 
+/* Cancel a pending abort work item.  smc_conn_abort_work() takes the socket
+ * lock, so the lock has to be dropped here.  Otherwise cancel_work_sync()
+ * waits for a worker that is itself blocked on the caller.  This is the idiom
+ * smc_close_cancel_work() already uses for close_work.
+ */
+static void smc_conn_cancel_abort_work(struct smc_connection *conn)
+{
+	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
+	struct sock *sk = &smc->sk;
+
+	if (current_work() == &conn->abort_work)
+		return;
+
+	release_sock(sk);
+	if (cancel_work_sync(&conn->abort_work))
+		sock_put(sk); /* sock_hold done by schedulers of abort_work */
+	lock_sock(sk);
+}
+
 /* remove a finished connection from its link group */
 void smc_conn_free(struct smc_connection *conn)
 {
@@ -1276,8 +1295,7 @@ void smc_conn_free(struct smc_connection *conn)
 			smcd_buf_detach(conn);
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
-		if (current_work() != &conn->abort_work)
-			cancel_work_sync(&conn->abort_work);
+		smc_conn_cancel_abort_work(conn);
 	}
 	if (!list_empty(&lgr->list)) {
 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
@@ -1750,7 +1768,12 @@ static void smc_conn_abort_work(struct work_struct *work)
 	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
 
 	lock_sock(&smc->sk);
-	smc_conn_kill(conn, true);
+	/* smc_conn_free() sets freed with this lock held and before it
+	 * releases anything, so a work item queued after the cancel has
+	 * nothing left to do.
+	 */
+	if (!conn->freed)
+		smc_conn_kill(conn, true);
 	release_sock(&smc->sk);
 	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
 }

base-commit: e9abfc6803fcd57ecca1a647638df773b6429eb9
-- 
2.52.0

Re: [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()

[Dust Li]

On 2026-09-07 13:47:21, Hidayath Khan wrote:
>smc_conn_free() disposes of a pending conn->abort_work, but it gets three
>things wrong:
>
>1. Deadlock: smc_conn_free() runs with the socket lock held and calls
>   cancel_work_sync(), while smc_conn_abort_work() takes the same lock.
>   If the work has already started on another CPU and is waiting for that
>   lock, the cancel waits for the work and the work waits for the caller.
>   The current_work() test only stops the work from cancelling itself,
>   not when the two run on different CPUs.
>
>2. Reference leak: Schedulers of abort_work take a socket reference, and
>   smc_conn_abort_work() drops it when it runs. If cancel_work_sync()
>   removes a pending work item before it runs, that reference is never
>   returned and the socket is never freed.
>
>Both are fixed the way smc_close_cancel_work() handles close_work: drop
>the socket lock around the cancel, and release the reference when the
>cancel reports that it removed a pending item.
>
>3. Late-queued work race: smc_cdc_rx_handler() finds the connection and
>   drops lgr->conns_lock before smc_cdc_msg_validate() decides to queue:
>
>     CPU0 (smc_conn_free)              CPU1 (smc_cdc_rx_handler)
>                                       conn = smc_lgr_find_conn()
>                                       sock_hold()
>                                       read_unlock_bh(&lgr->conns_lock)
>     cancel_work_sync()                  /* nothing queued yet */
>     smc_buf_unuse()
>                                       smc_cdc_msg_validate()
>                                         queue_work(&conn->abort_work)
>
>   cancel_work_sync() only guarantees that the work is not pending or
>   running when it returns; a racing enqueue lands after that. The work
>   then calls smc_conn_kill() on a connection whose buffers have already
>   been returned.
>
>Nothing smc_conn_free() does can prevent that enqueue, because the
>receiver already holds the connection pointer. Make the late work
>harmless instead: smc_conn_free() sets conn->freed with the socket lock
>held before it releases anything, and smc_conn_abort_work() takes the same
>lock. Check conn->freed inside smc_conn_abort_work() to skip
>smc_conn_kill() if teardown has started. The work still drops its socket
>reference.
>
>Fixes: b286a0651e44 ("net/smc: handle incoming CDC validation message")
>Cc: stable@vger.kernel.org
>Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>

Hi Hidayath,

Thanks for the fix. I think this is the right fix, and we can go ahead
and fix it this way for now.

Mahanta's smc_diag dump fix may need some rework on top of this one.

I believe the refactor I discussed with Mahanta in another thread would
address the root cause of these bugs more cleanly:
https://lore.kernel.org/netdev/apriDec7yHBkQgNv@linux.alibaba.com/

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

Best regards,
Dust