Re: [PATCH] net/smc: fix possible recursive locking in __smc_lgr_terminate()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/smc: self-deadlock in __smc_lgr_terminate() masked by lock_sock_nested()
- [Medium] net/smc: lockdep subclass inversion and unannotated relock in smc_conn_kill()
--

commit 7303e5e7071cf52d837c7140d6a0e1615372a594
Author: Anfernee Cao <anferneemelokc5@gmail.com>

net/smc: fix possible recursive locking in __smc_lgr_terminate()

This commit replaces lock_sock() with lock_sock_nested() in
__smc_lgr_terminate() to suppress a lockdep recursive locking warning when
smc_listen_work() aborts a connection and cleans up the link group early.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 6021322253e93..64c5ee592e641 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1579,7 +1579,7 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft)
>  		smc = container_of(conn, struct smc_sock, conn);
>  		sock_hold(&smc->sk); /* sock_put below */
>  		read_unlock_bh(&lgr->conns_lock);
> -		lock_sock(&smc->sk);
> +		lock_sock_nested(&smc->sk, SINGLE_DEPTH_NESTING);

[Severity: High]
Can this mask a real self-deadlock on new_smc->sk rather than a false
positive?

The commit description assumes smc_conn_free() always unregisters new_smc
from lgr->conns_all before smc_conn_abort() calls smc_lgr_cleanup_early().
However, in net/smc/smc_core.c, smc_conn_free() only unregisters the
connection if lgr->list is non-empty:

net/smc/smc_core.c:smc_conn_free() {
	...
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
}

When smc_listen_work() in net/smc/af_smc.c holds lock_sock(&new_smc->sk) and
waits for a confirm message via smc_clc_wait_msg(), a peer decline message
with the first-contact flag set causes smc_clc_wait_msg() in
net/smc/smc_clc.c to schedule link group termination before returning:

net/smc/smc_clc.c:smc_clc_wait_msg() {
	...
		if ((dclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK) &&
		    smc->conn.lgr) {
			smc->conn.lgr->sync_err = 1;
			smc_lgr_terminate_sched(smc->conn.lgr);
		}
	...
}

smc_lgr_terminate_sched() runs list_del_init(&lgr->list) and queues
lgr->terminate_work asynchronously while lgr->terminating is still 0.

When smc_listen_work() then calls
smc_listen_decline() -> smc_conn_abort() -> smc_conn_free(),
list_empty(&lgr->list) is already true, so smc_lgr_unregister_conn(conn) is
skipped and new_smc->conn remains in lgr->conns_all:

net/smc/af_smc.c:smc_conn_abort() {
	...
	smc_conn_free(conn);
	if (local_first && lgr_valid)
		smc_lgr_cleanup_early(lgr);
}

If smc_lgr_cleanup_early() -> __smc_lgr_terminate() runs before
lgr->terminate_work sets lgr->terminating, rb_first(&lgr->conns_all) returns
new_smc->conn itself. Using lock_sock_nested() here hides the recursive lock
acquisition from lockdep by assigning subclass 1 while the current task
already holds &new_smc->sk at subclass 0, causing the worker to hang in
__lock_sock(). The same sequence also applies to client first-contact setup
in smc_connect_rdma() and smc_connect_ism().

>  		smc_conn_kill(conn, soft);

[Severity: Medium]
Even when __smc_lgr_terminate() locks a different socket on the link group,
does using subclass 1 here invert the existing lockdep subclass hierarchy and
leave unannotated relocks inside smc_conn_kill()?

In net/smc/af_smc.c, smc_release() locks listening sockets with subclass 1
before smc_close_active() -> smc_close_cleanup_listen() ->
smc_close_non_accepted() locks child sockets at subclass 0:

net/smc/af_smc.c:smc_release() {
	...
	if (sk->sk_state == SMC_LISTEN)
		/* smc_close_non_accepted() is called and acquires
		 * sock lock for child sockets again
		 */
		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
	else
		lock_sock(sk);
	...
}

Locking a connection socket with subclass 1 in __smc_lgr_terminate() while
smc_listen_work() holds new_smc->sk at subclass 0 establishes the reverse
0 -> 1 ordering.

In addition, smc_conn_kill() calls smc_close_active_abort() in
net/smc/smc_close.c, which drops the socket lock and re-acquires it at
subclass 0 via smc_close_cancel_work() and when releasing clcsock:

net/smc/smc_close.c:smc_close_cancel_work() {
	struct sock *sk = &smc->sk;

	release_sock(sk);
	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);
	if (cancel_delayed_work_sync(&smc->conn.tx_work))
		sock_put(sk);
	lock_sock(sk);
}

net/smc/smc_close.c:smc_close_active_abort() {
	...
	if (release_clcsock) {
		release_sock(sk);
		smc_clcsock_release(smc);
		lock_sock(sk);
	}
	...
}

Wouldn't those plain lock_sock(sk) calls still trigger the recursive locking
warning against new_smc->sk when killing another active connection in the
link group?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179290097759.51540.15412712294727302463@gmail.com?part=1