* Re: [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()
2026-09-07 11:47 [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free() Hidayath Khan
@ 2026-09-08 3:22 ` Dust Li
2026-09-08 11:47 ` sashiko-bot
2026-09-09 11:49 ` netdev-bot+sashiko
2 siblings, 0 replies; 6+ messages in thread
From: Dust Li @ 2026-09-08 3:22 UTC (permalink / raw)
To: Hidayath Khan, alibuda, sidraya, mjambigi, andrew+netdev
Cc: tonylu, guwen, davem, edumazet, kuba, pabeni, horms, pasic,
linux-s390, netdev, linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()
2026-09-07 11:47 [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free() Hidayath Khan
2026-09-08 3:22 ` Dust Li
@ 2026-09-08 11:47 ` sashiko-bot
2026-09-09 6:48 ` Hidayath Khan
2026-09-09 11:49 ` netdev-bot+sashiko
2 siblings, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-09-08 11:47 UTC (permalink / raw)
To: Hidayath Khan
Cc: Christian Borntraeger, Vasily Gorbik, Alexander Gordeev,
linux-s390, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Late-queued `tx_work` can execute on a freed socket because it does not hold a socket reference, causing a use-after-free.
--
commit ea8d2d358addfe3a2e07acf0170476ee1325ff01
Author: Hidayath Khan <hidayath@linux.ibm.com>
net/smc: fix abort_work termination in smc_conn_free()
This commit addresses deadlocks, reference leaks, and late-queued work races
related to abort_work in smc_conn_free(). It drops the socket lock around
cancel_work_sync() and uses a freed flag to prevent late-queued work from
processing a destroyed 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 04aedd957543a..9a109eae73b92 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
[ ... ]
> @@ -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);
[Severity: Critical]
This is a pre-existing issue, but while fixing the exact same design flaw for
abort_work, does this leave tx_work vulnerable to a similar late-queue race?
If a local user closes an SMC socket, smc_close_cancel_work() cancels tx_work.
However, before the socket is fully destroyed, a remote peer sending a CDC
message (or a local user reading spliced data triggering
smc_rx_pipe_buf_release()) can cause the incoming message handler
smc_cdc_msg_recv_action() to call smc_tx_pending(). This can re-queue the work
after it was cancelled:
net/smc/smc_tx.c:smc_tx_consumer_update() {
...
queue_delayed_work(conn->lgr->tx_wq, &conn->tx_work, SMC_TX_WORK_DELAY);
...
}
Because tx_work does not hold a socket reference, it can be queued late and
outlive the socket. If the socket destruction completes and frees the socket,
and then tx_work expires and executes, it will dereference the freed socket
memory:
net/smc/smc_tx.c:smc_tx_work() {
...
lock_sock(&smc->sk);
...
}
Could this lead to a use-after-free if it is reliably reachable by a local
unprivileged user via splice() or by a remote peer sending timed CDC messages
during socket teardown?
> }
> if (!list_empty(&lgr->list)) {
> smc_buf_unuse(conn, lgr); /* allow buffer reuse */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907114721.1303498-1-hidayath@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()
2026-09-07 11:47 [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free() Hidayath Khan
2026-09-08 3:22 ` Dust Li
2026-09-08 11:47 ` sashiko-bot
@ 2026-09-09 11:49 ` netdev-bot+sashiko
2 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 11:49 UTC (permalink / raw)
To: hidayath
Cc: alibuda, dust.li, sidraya, mjambigi, andrew+netdev, tonylu, guwen,
davem, edumazet, kuba, pabeni, horms, pasic, linux-s390, netdev,
linux-rdma
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 0
- [High] New lock-drop window in smc_conn_free() invalidates the
smc_cdc_wait_pend_tx_wr() barrier before the connection buffers/link…
- [Medium] smc_conn_free() silently changes its locking contract without
documenting or asserting it, and its callers were not audited for the…
--
net/smc: fix abort_work termination in smc_conn_free()
This moves the abort_work cancel in smc_conn_free() into a new helper
smc_conn_cancel_abort_work() which drops the socket lock around
cancel_work_sync() and drops the socket reference when a pending item was
removed. It also makes smc_conn_abort_work() skip smc_conn_kill() when
conn->freed is already set.
Link: https://lore.kernel.org/netdev/20260806081549.595001-1-hidayath@linux.ibm.com/
A couple of questions about the new lock drop below.
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543a..9a109eae73b92 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;
> +
[Severity: Medium]
Should the new "caller must hold the socket lock" requirement be documented
or asserted? smc_conn_free() is declared cross-file in net/smc/smc_core.h
as
void smc_conn_free(struct smc_connection *conn);
and its comment still reads only "remove a finished connection from its
link group", yet for lgr->is_smcd == false it now unlocks and relocks the
socket. There is no sock_owned_by_me() or lockdep_assert_held() to catch a
caller that does not hold it.
All current call sites do hold the lock (__smc_release(),
smc_close_active_abort(), smc_close_passive_work(), smc_conn_abort()), so
nothing is unbalanced today. A backport or a new error path that reaches
smc_conn_free() without the lock would call release_sock() on a lock it does
not own and then return with the lock held.
There is also a caller-visible half. The tree's idiom around a
lock-dropping cancel revalidates state afterwards, as in
smc_close_active_abort():
sk->sk_state = SMC_PEERABORTWAIT;
smc_close_cancel_work(smc);
if (sk->sk_state != SMC_PEERABORTWAIT)
break;
sk->sk_state = SMC_CLOSED;
smc_conn_free(&smc->conn);
release_clcsock = true;
sock_put(sk); /* passive closing */
The smc_conn_free() calls in smc_close_active_abort(),
smc_close_passive_work() and __smc_release() revalidate nothing and act on
state that was observed before the new window (release_clcsock = true, the
"passive closing" sock_put()).
Does this also invalidate the invariant declared in smc_listen_work()?
lock_sock(&new_smc->sk); /* release in smc_listen_out() */
On an SMC-R handshake abort, smc_listen_work() -> smc_conn_abort() ->
smc_conn_free() now drops that lock in the middle, so another thread (for
example __smc_lgr_terminate(), which does lock_sock() before
smc_conn_kill()) can run against a half-set-up socket.
The changelog describes the change as a local fix to the cancel; would it be
worth stating that the calling convention of smc_conn_free() changed for
every call site?
> + release_sock(sk);
> + if (cancel_work_sync(&conn->abort_work))
> + sock_put(sk); /* sock_hold done by schedulers of abort_work */
> + lock_sock(sk);
> +}
> +
[Severity: High]
Does dropping the socket lock here void the pending-TX drain that
smc_conn_free() performs immediately before this call?
The SMC-R branch of smc_conn_free() is now:
smc_cdc_wait_pend_tx_wr(conn); /* cdc_pend_tx_wr drains to 0 */
smc_conn_cancel_abort_work(conn); /* release_sock() ... lock_sock() */
...
smc_buf_unuse(conn, lgr); /* zeroes sndbuf, used = 0 */
smc_lgr_unregister_conn(conn);
release_sock() on an SMC socket is not a passive unlock. sk_prot->release_cb
is smc_release_cb(), and net/smc/af_smc.c has:
if (smc->conn.tx_in_release_sock) {
smc_tx_pending(&smc->conn);
smc->conn.tx_in_release_sock = false;
}
conn->tx_in_release_sock is set from BH in smc_cdc_tx_handler(), on the very
completion that lets smc_cdc_wait_pend_tx_wr() return, when the socket is
owned by the task:
if (sock_owned_by_user(&smc->sk))
conn->tx_in_release_sock = true;
else
smc_tx_pending(conn);
if (unlikely(wq_has_sleeper(&conn->cdc_pend_tx_wq)))
wake_up(&conn->cdc_pend_tx_wq);
So smc_tx_pending() -> smc_tx_sndbuf_nonempty() -> smcr_tx_sndbuf_nonempty()
can post RDMA writes out of conn->sndbuf_desc and a CDC send, re-incrementing
conn->cdc_pend_tx_wr, inside the window this patch opens.
smc_tx_sndbuf_nonempty() only gates on sk_err, an empty send queue, peer RMBE
space and:
if (conn->killed ||
conn->local_rx_ctrl.conn_state_flags.peer_conn_abort) {
rc = -EPIPE; /* connection being aborted */
goto out;
}
conn->freed is never consulted on that path.
Is there a reachable state where sk_err and conn->killed are both clear while
staged data is still present? Closing an SMC-R socket with SO_LINGER=0 or
from an exiting task returns from smc_close_stream_wait() immediately:
if (!timeout)
return;
if (!smc_tx_prepared_sends(&smc->conn))
return;
which leaves prepared sends outstanding and the state at SMC_PEERCLOSEWAIT1,
so __smc_release() does not free the connection. When the peer closes,
smc_close_passive_work() reaches
smc_conn_free(conn);
with sk_err == 0, conn->killed == 0 and data still staged.
After lock_sock() returns, smc_buf_unuse() does memzero_explicit() on the
send buffer and sets used = 0, returning the descriptor to the link-group
pool while the HCA may still be reading it, and smcr_link_put() /
smc_lgr_put() then run with cdc_pend_tx_wr non-zero, so a later
smc_cdc_tx_handler() completion dereferences pend->conn and the link WR
structures.
Note also that conn->tx_work is not cancelled here, while
smc_close_cancel_work() does cancel_delayed_work_sync(&conn->tx_work). A
tx_work armed from BH via mod_delayed_work() in smcr_tx_sndbuf_nonempty()
can take the socket lock in the same window and post the same way.
Before this patch cancel_work_sync() ran with the socket lock held, so no
lock-taking tx path could interpose between the drain and the buffer release.
Would it work to cancel abort_work (and tx_work) before
smc_cdc_wait_pend_tx_wr(), or to repeat the drain after lock_sock() returns
and gate smc_tx_pending() on conn->freed?
> /* 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 */
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907114721.1303498-1-hidayath%40linux.ibm.com
^ permalink raw reply [flat|nested] 6+ messages in thread