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 Signed-off-by: Hidayath Khan --- 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