Re: [PATCH AUTOSEL 6.18] nvme-tcp: delay nvme_tcp_reclassify_socket()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvme-tcp: unsafe live lock reinit in nvme_tcp_reclassify_socket()
--

commit 168d45f90ed1a7b47f62fbd5085e5e0bbb05d5a8
Author: Eric Dumazet <edumazet@google.com>

nvme-tcp: delay nvme_tcp_reclassify_socket()

This commit delays nvme_tcp_reclassify_socket() in nvme_tcp_alloc_queue()
until after kernel_connect() and the optional nvme_tcp_start_tls() handshake
complete, in order to avoid pulling tlshd lock dependencies into the
nvme-tcp socket lockdep class.

Link: https://lore.kernel.org/lkml/CANn89i+wnTLC==UnXCpjsS4YxvEfhe5oK0N7fttbqr1zKyqdug@mail.gmail.com/

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index beef2b576102a..da148092677fe 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1859,7 +1859,6 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  	}
>  
>  	sk_net_refcnt_upgrade(queue->sock->sk);
> -	nvme_tcp_reclassify_socket(queue->sock);
>  
>  	/* Single syn retry */
>  	tcp_sock_set_syncnt(queue->sock->sk, 1);
> @@ -1948,6 +1947,8 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  			goto err_init_connect;
>  	}
>  
> +	nvme_tcp_reclassify_socket(queue->sock);
> +

[Severity: High]
Is it safe to call nvme_tcp_reclassify_socket() here after kernel_connect()
and nvme_tcp_start_tls() have already run?

When CONFIG_DEBUG_LOCK_ALLOC is enabled, nvme_tcp_reclassify_socket() checks
sock_allow_reclassification() and calls sock_lock_init_class_and_name() in
process context with bottom halves enabled and no lock held:

drivers/nvme/host/tcp.c:nvme_tcp_reclassify_socket() {
	struct sock *sk = sock->sk;

	if (WARN_ON_ONCE(!sock_allow_reclassification(sk)))
		return;

	switch (sk->sk_family) {
	case AF_INET:
		sock_lock_init_class_and_name(sk, "slock-AF_INET-NVME",
					      &nvme_tcp_slock_key[0],
					      "sk_lock-AF_INET-NVME",
					      &nvme_tcp_sk_key[0]);
		break;
    ...
}

And sock_lock_init_class_and_name() reinitializes sk->sk_lock in-place:

include/net/sock.h:sock_lock_init_class_and_name()
	sk_owner_set(sk, THIS_MODULE);
	sk->sk_lock.owned = 0;
	init_waitqueue_head(&sk->sk_lock.wq);
	spin_lock_init(&(sk)->sk_lock.slock);
	debug_check_no_locks_freed((void *)&(sk)->sk_lock,
				   sizeof((sk)->sk_lock));
	lockdep_set_class_and_name(&(sk)->sk_lock.slock,
				   (skey), (sname));
	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);

At this point in nvme_tcp_alloc_queue() (reached from
nvme_tcp_alloc_admin_queue() or __nvme_tcp_alloc_io_queues()),
kernel_connect() has already transitioned queue->sock->sk to TCP_ESTABLISHED,
hashed it into inet_ehash, and armed TCP timers. In addition, when TLS is
used, handshake_nl_done_doit() -> handshake_complete() ->
nvme_tcp_tls_done() completes queue->tls_complete before userspace tlshd
closes its open file descriptor to queue->sock->file.

If incoming peer packets (such as TCP ACKs, window updates, TLS 1.3
post-handshake NewSessionTicket messages, or FIN/RST) trigger softirq TCP RX
(tcp_v4_rcv() / tcp_v6_rcv()), or if TCP timers (tcp_write_timer() /
tcp_delack_timer()) or tlshd activity coincide with
nvme_tcp_reclassify_socket():

1. Can softirq or tlshd acquire sk->sk_lock.slock or sk->sk_lock right after
   sock_allow_reclassification(sk) succeeds, racing with spin_lock_init(),
   init_waitqueue_head(), sk->sk_lock.owned = 0,
   debug_check_no_locks_freed(), lockdep_set_class_and_name(), and
   lockdep_init_map() to corrupt the spinlock, waitqueue, and lockdep state?

2. If softirq or tlshd already holds sk->sk_lock.slock or sk->sk_lock.owned
   when sock_allow_reclassification(sk) is checked, won't this trigger
   WARN_ON_ONCE(!sock_allow_reclassification(sk)) and abort reclassification,
   leaving the socket in the default lockdep class?

>  	ret = nvme_tcp_init_connection(queue);
>  	if (ret)
>  		goto err_init_connect;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-249-sashal@kernel.org?part=1