Re: [PATCH net] net/smc: drop the abort_work reference when the work is cancelled

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] AB-BA deadlock between `smc_conn_free()` and `smc_conn_abort_work()` caused by calling `cancel_work_sync()` while holding the socket lock.
--

--- Patch [1]: [PATCH net] net/smc: drop the abort_work reference when the work is cancelled ---
commit 1b6cdab59cd811b97297f9b5597cac1aa3e420d9
Author: Hidayath Khan <[email protected]>

net/smc: drop the abort_work reference when the work is cancelled

This commit fixes a socket reference leak that occurs when an abort_work
item is cancelled before it runs. It drops the socket reference if
cancel_work_sync() returns true, ensuring the socket can be properly
destroyed.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5e..3d4a86d99dd6b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
[ ... ]
> @@ -1275,8 +1276,13 @@ void smc_conn_free(struct smc_connection *conn)
>  		tasklet_kill(&conn->rx_tsklet);
>  	} else {
>  		smc_cdc_wait_pend_tx_wr(conn);
> -		if (current_work() != &conn->abort_work)
> -			cancel_work_sync(&conn->abort_work);
> +		/* If the work was pending (cancel returns true) it never ran,
> +		 * so the sock_hold taken by its scheduler was never released.
> +		 */
> +		if (current_work() != &conn->abort_work) {
> +			if (cancel_work_sync(&conn->abort_work))
> +				sock_put(&smc->sk);

[Severity: High]
This isn't a bug introduced by this patch, but can calling cancel_work_sync()
here cause an AB-BA deadlock with the workqueue thread?

If a thread initiates a socket close, such as via smc_release() or
smc_close_passive_work(), it acquires the socket lock:

smc_release()
  lock_sock(&smc->sk);
  ...
  smc_conn_free()
    cancel_work_sync(&conn->abort_work);

If a remote peer concurrently sends an out-of-order CDC message, abort_work
is queued. The workqueue thread starts executing smc_conn_abort_work() and
attempts to acquire the same socket lock:

smc_conn_abort_work()
  lock_sock(&smc->sk);
  smc_conn_kill(conn, true);
  release_sock(&smc->sk);

Could this result in the workqueue thread blocking on the socket lock, while
the socket release thread blocks indefinitely in cancel_work_sync() waiting
for the workqueue thread to finish?

> +		}
>  	}
>  	if (!list_empty(&lgr->list)) {
>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1