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

[Dust Li <dust.li@linux.alibaba.com>]

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