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

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

[Anfernee Cao]

syzbot reports a possible recursive locking in the SMC listen path:

  WARNING: possible recursive locking detected
    kworker/0:2 is trying to acquire lock: sk_lock-AF_SMC
      at __smc_lgr_terminate() (net/smc/smc_core.c:1580)
    but task is already holding lock: sk_lock-AF_SMC
      at smc_listen_work() (net/smc/af_smc.c:2463)

smc_listen_work() holds new_smc->sk while calling
smc_listen_decline() -> smc_conn_abort() -> smc_lgr_cleanup_early().
smc_conn_free() unregisters new_smc from lgr->conns_all before the
early cleanup, so __smc_lgr_terminate() walks the remaining
connections and locks another socket, not the one already held.

All sockets use the same sk_lock class, so lockdep cannot tell the
two instances apart and reports a possible recursive acquisition.
Annotate the per-connection lock as a nested acquisition, as
smc_listen_out() already does for the listener socket.

Reported-by: syzbot+7ea1f5efd5de7e24f1de@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7ea1f5efd5de7e24f1de
Fixes: 2c7f14ed9c19 ("net/smc: fix LGR and link use-after-free issue")
Cc: stable@vger.kernel.org # v6.13+
Signed-off-by: Anfernee Cao <anferneemelokc5@gmail.com>
---
 net/smc/smc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..5911368ae520 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1577,7 +1577,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);
 		smc_conn_kill(conn, soft);
 		release_sock(&smc->sk);
 		sock_put(&smc->sk); /* sock_hold above */
-- 
2.54.0.windows.1

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message says smc_conn_free() always unregisters
  new_smc from lgr->conns_all before early cleanup, so…
- [High] The new annotation in __smc_lgr_terminate()
  (net/smc/smc_core.c:1582) creates the lockdep dependency sk_lock-…
- [High] The new nesting annotation covers only the first acquisition of
  the per-connection socket in __smc_lgr_terminate()…

--

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

The patch switches the per-connection lock_sock() in __smc_lgr_terminate()
to lock_sock_nested(..., SINGLE_DEPTH_NESTING). The goal is to silence a
syzbot lockdep "possible recursive locking" report in the SMC listen path.
The commit message argues that the socket locked there is always a
different instance from the one the caller already holds.

> 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]
The commit message says:

  smc_conn_free() unregisters new_smc from lgr->conns_all before the
  early cleanup, so __smc_lgr_terminate() walks the remaining
  connections and locks another socket, not the one already held.

Is that always true? smc_conn_free() only unregisters the conn while
lgr->list is still 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);
	}
    ...
}

Suppose the peer answers ACCEPT with a CLC DECLINE that has
SMC_FIRST_CONTACT_MASK set. That arrives while smc_listen_work() is
waiting for CONFIRM and still holding new_smc->sk:

smc_listen_work()
  lock_sock(&new_smc->sk)
  smc_clc_wait_msg()
    smc_lgr_terminate_sched(smc->conn.lgr)
      list_del_init(&lgr->list);
      lgr->freeing = 1;
      schedule_work(&lgr->terminate_work);
  smc_listen_decline()
    smc_conn_abort()
      smc_conn_free()        <- lgr->list empty, conn stays in conns_all
      smc_lgr_cleanup_early()
        __smc_lgr_terminate()
          lock_sock_nested(&new_smc->sk, SINGLE_DEPTH_NESTING)

smc_lgr_terminate_sched() does not set lgr->terminating. If
terminate_work has not run yet, __smc_lgr_terminate() gets past its
terminating check, and rb_first(&lgr->conns_all) can return new_smc's own
conn.

smc_conn_abort() computes lgr_valid before it calls smc_conn_free(), so
the early cleanup still runs on this path.

Wouldn't the kworker then sleep forever in __lock_sock(), waiting for a
socket lock it already owns?

lock_sock() is just lock_sock_nested(sk, 0), so this change doesn't
alter runtime behavior. It only changes the lockdep subclass, which hides
the self-recursion report in this case.

Would the fix need to make sure the caller's own conn is unregistered, or
skipped in __smc_lgr_terminate(), rather than annotating the lock?

[Severity: High]
Can this annotation create a lockdep ordering inversion against
smc_release()?

With this patch, the listen and connect paths record
sk_lock-AF_SMC -> sk_lock-AF_SMC/1:

smc_listen_work()
  lock_sock(&new_smc->sk)                              subclass 0
  smc_listen_decline()->smc_conn_abort()->smc_lgr_cleanup_early()
    __smc_lgr_terminate()
      lock_sock_nested(&smc->sk, SINGLE_DEPTH_NESTING) subclass 1

smc_release() already records the opposite order when it closes a
listener that still has non-accepted children:

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);
    ...
}

smc_release()
  lock_sock_nested(sk, SINGLE_DEPTH_NESTING)           listener, subclass 1
  __smc_release()->smc_close_active()->smc_close_cleanup_listen()
    smc_close_non_accepted()
      lock_sock(sk)                                    child, subclass 0

Lockdep keeps these dependencies for the whole boot. Once both orders
have been seen, wouldn't it report a possible circular locking dependency
between sk_lock-AF_SMC and sk_lock-AF_SMC/1, triggered by the same syzbot
scenario?

The sockets involved are different instances, so this may not be a real
deadlock. The splat would still fire, though, and lockdep would turn
itself off afterwards.

[Severity: High]
Does the nested annotation cover the whole time this socket lock is held?

smc_conn_kill() calls smc_close_active_abort(). For SMC_ACTIVE,
SMC_APPCLOSEWAIT1/2, SMC_PEERCLOSEWAIT1/2, SMC_PEERFINCLOSEWAIT,
SMC_PROCESSABORT and SMC_APPFINCLOSEWAIT, that goes through
smc_close_cancel_work():

net/smc/smc_close.c:smc_close_cancel_work() {
	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);
}

The release_clcsock branch at the end of smc_close_active_abort() also
takes the lock again with a plain lock_sock(sk).

Both of these take the conn socket again at subclass 0, while
smc_listen_work() still holds new_smc at subclass 0. Wouldn't lockdep
then report the same "possible recursive locking detected", this time
from smc_close_cancel_work()?

This looks reachable for SMC-D, because smc_listen_work() drops
smc_server_lgr_pending right after sending ACCEPT:

	/* SMC-D does not need this lock any more */
	if (ini->is_smcd)
		mutex_unlock(&smc_server_lgr_pending);

Another connection can then join the first-contact lgr and become
SMC_ACTIVE. If this handshake later fails, for example on a CONFIRM
timeout, a failed smc_clc_v2x_features_confirm_check() or a failed
smcd_buf_attach(), this path kills the SMC_ACTIVE conn:

smc_listen_decline()->smc_conn_abort()->smc_lgr_cleanup_early()
  ->__smc_lgr_terminate()

>  		smc_conn_kill(conn, soft);
>  		release_sock(&smc->sk);
>  		sock_put(&smc->sk); /* sock_hold above */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179290097759.51540.15412712294727302463%40gmail.com