[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

Re: [PATCH net v2] 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:

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

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

[Hidayath Khan]

On 08/09/26 5:17 pm, sashiko-bot@kernel.org wrote:
> 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.
> --
Thanks for the Sashiko AI review. The finding is correct.

tx_work is the only one of the three connection work items with no socket
reference:

   close_work   sock_hold smc_cdc.c:420   sock_put smc_close.c:444
   abort_work   sock_hold smc_cdc.c:324   sock_put smc_core.c:1754
   tx_work        none                                  none

The link group holds a reference while the connection is registered, which
is what hides this. smc_conn_free() drops it in smc_lgr_unregister_conn()
and never cancels tx_work - it only cancels abort_work - and the link
group usually outlives the socket, so lgr->tx_wq is still there to run an
item that was queued late.

Worth noting the fix in this patch does not transfer. conn->freed works
for abort_work because abort_work holds a reference, so the socket is
alive and the flag is safe to read. tx_work holds nothing, so by the time
it runs there may be no conn to test. It needs the missing reference
rather than a guard: sock_hold() at the three queue sites, sock_put() in
smc_tx_work(), and the reference returned at the cancel sites that report
they removed a pending item.

I will send that separately, since it is a different work item and a
different fix. This patch is unchanged.

Thanks,
Hidayath
>
> 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 */

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

[Paolo Abeni]

On 9/9/26 8:48 AM, Hidayath Khan wrote:
> 
> On 08/09/26 5:17 pm, sashiko-bot@kernel.org wrote:
>> 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.
>> --
> Thanks for the Sashiko AI review. The finding is correct.
> 
> tx_work is the only one of the three connection work items with no socket
> reference:
> 
>    close_work   sock_hold smc_cdc.c:420   sock_put smc_close.c:444
>    abort_work   sock_hold smc_cdc.c:324   sock_put smc_core.c:1754
>    tx_work        none                                  none
> 
> The link group holds a reference while the connection is registered, which
> is what hides this. smc_conn_free() drops it in smc_lgr_unregister_conn()
> and never cancels tx_work - it only cancels abort_work - and the link
> group usually outlives the socket, so lgr->tx_wq is still there to run an
> item that was queued late.
> 
> Worth noting the fix in this patch does not transfer. conn->freed works
> for abort_work because abort_work holds a reference, so the socket is
> alive and the flag is safe to read. tx_work holds nothing, so by the time
> it runs there may be no conn to test. It needs the missing reference
> rather than a guard: sock_hold() at the three queue sites, sock_put() in
> smc_tx_work(), and the reference returned at the cancel sites that report
> they removed a pending item.
> 
> I will send that separately, since it is a different work item and a
> different fix. This patch is unchanged.
Could you please have a look at clashiko report, too? that looks
correct, too and more strictly tied to the issue addressed here.

Also note that netdev-wise the most relevant sashiko instance is
the nipa/clashiko one, as it includes reports/reviews/findings
from the public clashiko instance.

/P

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

[Hidayath Khan]

On 10/09/26 4:20 pm, Paolo Abeni wrote:
> On 9/9/26 8:48 AM, Hidayath Khan wrote:
>> On 08/09/26 5:17 pm, sashiko-bot@kernel.org wrote:
>>> 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.
>>> --
>> Thanks for the Sashiko AI review. The finding is correct.
>>
>> tx_work is the only one of the three connection work items with no socket
>> reference:
>>
>>     close_work   sock_hold smc_cdc.c:420   sock_put smc_close.c:444
>>     abort_work   sock_hold smc_cdc.c:324   sock_put smc_core.c:1754
>>     tx_work        none                                  none
>>
>> The link group holds a reference while the connection is registered, which
>> is what hides this. smc_conn_free() drops it in smc_lgr_unregister_conn()
>> and never cancels tx_work - it only cancels abort_work - and the link
>> group usually outlives the socket, so lgr->tx_wq is still there to run an
>> item that was queued late.
>>
>> Worth noting the fix in this patch does not transfer. conn->freed works
>> for abort_work because abort_work holds a reference, so the socket is
>> alive and the flag is safe to read. tx_work holds nothing, so by the time
>> it runs there may be no conn to test. It needs the missing reference
>> rather than a guard: sock_hold() at the three queue sites, sock_put() in
>> smc_tx_work(), and the reference returned at the cancel sites that report
>> they removed a pending item.
>>
>> I will send that separately, since it is a different work item and a
>> different fix. This patch is unchanged.
> Could you please have a look at clashiko report, too? that looks
> correct, too and more strictly tied to the issue addressed here.
Yes, the findings are correct. I will look into the report.
>
> Also note that netdev-wise the most relevant sashiko instance is
> the nipa/clashiko one, as it includes reports/reviews/findings
> from the public clashiko instance.
Noted, thanks!
>
> /P

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

[netdev-bot+sashiko]

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

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

[Hidayath Khan]

On 09/09/26 5:19 pm, netdev-bot+sashiko@kernel.org wrote:
> 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…
>
> --
Thanks for the review. Both the findings are correct, and together they rule
out the approach v2 took rather than asking for an adjustment to it, so v3
drops the lock-drop entirely.
>
> 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?

Yes. This is the finding that ended the approach, and the ordering makes it
the natural interleaving rather than a narrow race. In smc_cdc_tx_handler():

     if (atomic_dec_and_test(&conn->cdc_pend_tx_wr)) {
         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);
     }

conn->tx_in_release_sock is set before the wake_up, on the very completion
that lets smc_cdc_wait_pend_tx_wr() return, and the socket is owned by the
task doing the teardown. So the drain returns with the flag already set,
and the release_sock() immediately after it runs smc_release_cb() ->
smc_tx_pending() -> smc_tx_sndbuf_nonempty(), which posts RDMA writes out of
conn->sndbuf_desc and re-increments conn->cdc_pend_tx_wr. smc_buf_unuse()
then memzero_explicit()s that buffer and sets used = 0, returning the
descriptor to the link group pool - it never clears conn->sndbuf_desc - so
another connection can be handed a buffer the adapter is still reading, and
smcr_link_put()/smc_lgr_put() run with cdc_pend_tx_wr non-zero.
>
> 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?
Cancelling before the drain does not help, because the problem is not the
order of the cancel and the drain - it is that release_sock() on an SMC
socket runs the tx path at all. Moving the cancel earlier moves the window,
it does not close it: the lock is still dropped between the drain and
smc_buf_unuse().

Repeating the drain after lock_sock() has the same shape as the first one.
The completion that ends the second drain can set tx_in_release_sock too.

v3 removes the window: the socket lock is never dropped. smc_conn_free()
sets conn->freed under the socket lock before it releases anything,
and smc_conn_abort_work() takes the same lock, so an instance that is
running is parked on that lock and will find the flag set, and one queued
afterwards finds the same - and then the cancel no longer has to wait for
anything, so cancel_work() replaces cancel_work_sync(). Returning the
reference when it reports it removed a pending item fixes the leak v1
was about.
>
>>   /* 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 */
> [ ... ]
>