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

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

[Hidayath Khan]

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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/smc: zombie abort_work terminates valid fallback connection
--

commit cebb96f694baa10cd5a7d057d497a08031c69485
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: fix abort_work termination in smc_conn_free()

This commit switches smc_conn_free() to use a non-blocking cancel_work()
for abort_work to avoid deadlocks. It also adds a check for conn->freed
inside smc_conn_abort_work() and initializes abort_work once per socket
in smc_sk_init() instead of per connection.

Link: https://lore.kernel.org/netdev/20260806081549.595001-1-hidayath@linux.ibm.com/

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..907530e1d6464 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c

[ ... ]

> @@ -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);

[Severity: High]
Does changing this to a non-blocking cancel_work() introduce a race with
server-side fallback connections?

During server listen, smc_listen_find_device() iterates over proposed
devices while holding the socket lock. If an initial connection attempt
calls smc_conn_create() but fails and is aborted via smc_conn_free(), an
abort_work queued concurrently could transition to running but block on the
socket lock.

Since cancel_work() does not wait, it would return false for the running
worker and leave it parked on the lock. When the fallback loop continues,
it calls smc_conn_create() for the next device on the same socket, which
unconditionally resets the flag:

net/smc/smc_core.c:smc_conn_create() {
    ...
    conn->freed = 0;
    ...
}

When the fallback connection ultimately succeeds and releases the socket
lock, could the parked abort_work unblock, observe the reset conn->freed
flag, and erroneously call smc_conn_kill() on the new, valid connection?

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006073550.1595003-1-hidayath@linux.ibm.com?part=1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Resetting conn->freed in smc_conn_create() re-arms a stale
  abort_work, so the new disarm scheme fails when one socket gets a…

Pre-existing issues:
- [High] smc_cdc_msg_validate() (net/smc/smc_cdc.c:319-323) sets
  conn->out_of_sync = 1, local_tx_ctrl.conn_state_flags.peer_conn_abort…

--

net/smc: fix abort_work termination in smc_conn_free()

The patch replaces cancel_work_sync() in smc_conn_free() with a
non-blocking cancel_work(). When cancel_work() removes a pending item, the
patch drops that item's socket reference. smc_conn_abort_work() now skips
smc_conn_kill() once conn->freed is set. INIT_WORK() for abort_work also
moves from smc_conn_create() to smc_sk_init().

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..907530e1d6464 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c

[ ... ]

> @@ -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 */

[ ... ]

> @@ -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);

[Severity: High]
Can a stale abort_work still run smc_conn_kill() on a later connection of
the same socket?

cancel_work() does not wait for an instance that is already running, and it
does not block a later requeue. That leaves conn->freed as the only thing
disarming the work. However, smc_conn_create() clears it unconditionally:

net/smc/smc_core.c:smc_conn_create() {
    ...
	smc_lgr_hold(conn->lgr); /* lgr_put in smc_conn_free() */
	if (!conn->lgr->is_smcd)
		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
	conn->freed = 0;
    ...
}

On the listen side, the socket can get a second connection while
smc_listen_work() still holds the socket lock:

smc_listen_work()
  smc_listen_find_device()
    smc_find_rdma_v2_device_serv()
      smc_listen_rdma_init()
        smc_conn_create()              /* conn A, maybe reused lgr */
      smc_listen_rdma_reg()
        smcr_lgr_reg_rmbs()
          smc_llc_do_confirm_rkey()    /* peer times out or RKEY_NEG */
      smc_conn_abort()
        smc_conn_free()                /* conn->freed = 1 */
    smc_find_rdma_v1_device_serv()     /* when prfx_rc == 0 */
      smc_listen_rdma_init()
        smc_conn_create()              /* conn B, conn->freed = 0 */

A failover_validation CDC for conn A can arrive in this window.
smc_cdc_rx_handler() looks up conn A, drops lgr->conns_lock, and
smc_cdc_msg_validate() queues abort_work. That work is either parked in
lock_sock() or queued after the cancel_work() in smc_conn_free(A).

In both cases it can only proceed after smc_listen_out() releases the socket
lock. By then conn->freed is 0 again, so smc_conn_kill(conn, true) runs on
conn B. That sends an abort, unregisters conn B and closes the socket.

smc_conn_create() also never clears conn->out_of_sync or
local_tx_ctrl.conn_state_flags.peer_conn_abort. smc_cdc_msg_validate() set
both for conn A, so does conn B inherit them? If so, every CDC for conn B
would be dropped by the out_of_sync check in smc_cdc_rx_handler().

Triggering this needs a validation CDC with conn A's alert token before the
CLC accept is sent. So it would take a misbehaving SMC-R peer that shares an
existing link group, rather than a well-behaved one.

Before this patch, the same sequence ran into cancel_work_sync() or re-ran
INIT_WORK() on a pending item. So this looks like a gap in the fix rather
than something new compared to the baseline.

The commit message says:

  "An instance that is running is parked on that lock and will find the
  flag set; an instance queued afterwards finds the same."

The new comment above smc_conn_free() also says conn->freed "disarms a
pending or running abort_work".

Do these statements still hold when the connection is re-created on the same
socket? The commit message itself notes that smc_listen_find_device()
creates a connection once per device it tries.

Would a per-lifetime guard be needed here instead of the freed flag alone?
For example, a generation number recorded when the work is queued and
checked in smc_conn_abort_work(). Another option would be to reset the abort
state and drain safely before re-creating the connection.

>  	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 */

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The same
late-receiver window also lets smc_cdc_msg_validate() write connection
state directly, without the socket lock and without taking link references:

net/smc/smc_cdc.c:smc_cdc_msg_validate() {
    ...
		conn->out_of_sync = 1;	/* prevent any further receives */
		spin_lock_bh(&conn->send_lock);
		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
		conn->lnk = link;
		spin_unlock_bh(&conn->send_lock);
    ...
}

smc_conn_create() and smc_conn_free() set and use conn->lnk under
lock_sock() and never take conn->send_lock.

Suppose a receiver that found conn A in smc_cdc_rx_handler() does these
writes after smc_conn_free(A) and smc_conn_create(B), via the v2 -> v1
device search above. Could conn B then end up with conn->lnk pointing at a
link in conn A's link group that it never held?

If so, __smc_lgr_unregister_conn() would decrement the wrong link's
conn_cnt, and smc_conn_free(B) would then run:

	smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */

on a link with no matching hold. Can that drop the link's refcount to zero
early and run __smcr_link_clear() while the link is still in use? The hold
on conn B's real link would also be leaked, and its link group never freed.

The race window is narrow, but it is driven by input from the remote link
peer.

The race aside, changing conn->lnk here skips the pairing that
smc_switch_link_and_count() uses:

	atomic_dec(&conn->lnk->conn_cnt);
	/* link_hold in smc_conn_create() */
	smcr_link_put(conn->lnk);
	conn->lnk = to_lnk;
	atomic_inc(&conn->lnk->conn_cnt);
	/* link_put in smc_conn_free() */
	smcr_link_hold(conn->lnk);

Does a validation CDC that arrives on a link other than conn->lnk unbalance
the link refcounts as well?

The patch only makes the work item harmless. These direct writes from the
receiver are still reachable in the window the commit message describes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006073550.1595003-1-hidayath%40linux.ibm.com