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

[Hidayath Khan <hidayath@linux.ibm.com>]

smc_conn_free() cancels a pending conn->abort_work, and that cancel is
wrong in three ways.

It deadlocks. smc_conn_free() runs with the socket lock held, and
smc_conn_abort_work() takes the same lock, so cancel_work_sync() against an
instance that has already started waits for a worker that is waiting for
the caller.  The current_work() test only stops the work cancelling itself.
The lock cannot simply be dropped around the cancel either: several
smc_conn_abort() paths hold smc_client_lgr_pending or
smc_server_lgr_pending, which is taken after the socket lock, and
release_sock() on an SMC socket runs smc_release_cb(), which can post from
conn->sndbuf_desc again.

It leaks a socket reference. Schedulers of abort_work take one and
smc_conn_abort_work() returns it when it runs, so an item that
cancel_work_sync() removes before it runs never gives its reference back.

It does not stop the work being queued again. smc_cdc_rx_handler() takes
its socket reference and leaves lgr->conns_lock before
smc_cdc_msg_validate() decides to queue, so a receiver already past that
unlock can queue after the cancel has returned. cancel_work_sync() only
promises that the work is neither pending nor running when it returns.

Make the work harmless first: smc_conn_free() sets conn->freed with the
socket lock held and before it releases anything, and the work takes the
same lock, so testing the flag there is exact. An instance that is running
is parked on that lock and will find the flag set; an instance queued
afterwards finds the same.

The cancel can then be asynchronous. cancel_work() never waits, so the
deadlock is gone and the socket lock is never dropped, and returning the
reference when it reports that it removed a pending item fixes the leak.

Testing the flag also covers the early exit that no cancel ever reached.
conn->freed is set before the smc_conn_lgr_valid() test, so the goto
lgr_put path is covered too. That path is taken when smc_conn_kill() has
already unregistered the connection and then reaches smc_conn_free()
through smc_close_active_abort(); __smc_lgr_terminate() goes on to
smc_lgr_free(), whose smc_lgr_put() can drop the last reference and
kfree() the link group, while a queued abort_work would still have
dereferenced conn->lgr in smc_conn_kill().

Finally, move INIT_WORK() out of smc_conn_create(). abort_work is the only
per connection work item initialised per connection rather than once per
socket, and that asymmetry is what makes the cancel above have to reason
about re-initialisation at all: smc_listen_find_device() creates a
connection once per device it tries, so a socket can run INIT_WORK() on
this work item several times. Initialise it where tx_work and close_work
are already initialised, and the work item's lifetime matches the socket's.

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>
---
v3:
- Keep the socket lock held continuously during smc_conn_free() instead of
  dropping it. Dropping the lock in v2 had two issues:
  1. Inverted lock ordering: smc_conn_abort() runs inside
     smc_{client,server}_lgr_pending on listen/connect paths, so releasing
     the socket lock violates the socket lock -> mutex hierarchy.
  2. Spurious TX posting: release_sock() invokes smc_release_cb(), which
     can trigger smc_tx_pending() and post from conn->sndbuf_desc after
     the drain and buffer release.
- Switch from cancel_work_sync() to non-blocking cancel_work():
  - If the work was pending, drop its socket reference immediately.
  - If the work is already running, it waits on our socket lock and will
    safely no-op upon seeing conn->freed once the lock is released.
- Move INIT_WORK() from smc_conn_create() to smc_sk_init() so the work item
  is initialized once per socket lifetime, avoiding re-initialization races
  across multiple device searches in smc_listen_work().
- Document the socket lock requirement in smc_conn_free().
- Dropped Reviewed-by tag due to substantial implementation changes.

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/af_smc.c   |  1 +
 net/smc/smc_core.c | 25 +++++++++++++++++++------
 net/smc/smc_core.h |  1 +
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..5b1bee22fc59 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -404,6 +404,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
 	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
 	INIT_WORK(&smc->connect_work, smc_connect_work);
 	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
+	INIT_WORK(&smc->conn.abort_work, smc_conn_abort_work);
 	INIT_LIST_HEAD(&smc->accept_q);
 	sock_lock_init_class_and_name(sk, "slock-AF_SMC", &smc_slock_key,
 				      "sk_lock-AF_SMC", &smc_key);
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..907530e1d646 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1251,9 +1251,13 @@ static void smc_buf_unuse(struct smc_connection *conn,
 	}
 }
 
-/* remove a finished connection from its link group */
+/* remove a finished connection from its link group.
+ * Must be called with the socket lock held: conn->freed is what disarms
+ * a pending or running abort_work, and both are set and tested under it.
+ */
 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)
@@ -1276,8 +1280,13 @@ 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);
+		/* Do not wait here: the work takes the socket lock this
+		 * caller holds. An instance that is already running is
+		 * parked on that lock and will find conn->freed set; only a
+		 * still-pending one has to give its reference back.
+		 */
+		if (cancel_work(&conn->abort_work))
+			sock_put(&smc->sk);
 	}
 	if (!list_empty(&lgr->list)) {
 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
@@ -1742,7 +1751,7 @@ void smcr_lgr_set_type_asym(struct smc_link_group *lgr,
 }
 
 /* abort connection, abort_work scheduled from tasklet context */
-static void smc_conn_abort_work(struct work_struct *work)
+void smc_conn_abort_work(struct work_struct *work)
 {
 	struct smc_connection *conn = container_of(work,
 						   struct smc_connection,
@@ -1750,7 +1759,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 an instance that was queued or parked by
+	 * then 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 */
 }
@@ -2059,7 +2073,6 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 	conn->local_tx_ctrl.len = SMC_WR_TX_SIZE;
 	conn->urg_state = SMC_URG_READ;
 	init_waitqueue_head(&conn->cdc_pend_tx_wq);
-	INIT_WORK(&smc->conn.abort_work, smc_conn_abort_work);
 	if (ini->is_smcd) {
 		conn->rx_off = sizeof(struct smcd_cdc_msg);
 		smcd_cdc_rx_init(conn); /* init tasklet for this conn */
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 5c18f08a4c8a..52e3ba9f6d69 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -596,6 +596,7 @@ void smc_rmb_sync_sg_for_cpu(struct smc_connection *conn);
 int smc_vlan_by_tcpsk(struct socket *clcsock, struct smc_init_info *ini);
 
 void smc_conn_free(struct smc_connection *conn);
+void smc_conn_abort_work(struct work_struct *work);
 int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini);
 int smc_core_init(void);
 void smc_core_exit(void);

base-commit: a3726256fa7e8ad8a3ee4855cedc7a762e5d8252
-- 
2.52.0