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

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

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 */
> [ ... ]
>