Thread (21 messages) flat view 21 messages, 4 authors, 16d ago

Re: [syzbot] [net?] WARNING: locking bug in tcp_tsq_handler

From: Shin'ichiro Kawasaki <hidden>
Date: 2026-08-26 02:24:05
Also in: lkml

On Aug 25, 2026 / 16:27, Eric Dumazet wrote:
On Tue, Aug 25, 2026 at 3:53 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Aug 25, 2026 at 3:26 PM Shin'ichiro Kawasaki
[off-list ref] wrote:
quoted
On Aug 25, 2026 / 14:45, Hillf Danton wrote:
quoted
On Tue, 25 Aug 2026 14:44:35 +0900 Shin'ichiro Kawasaki wrote:
quoted
On Aug 25, 2026 / 03:50, Eric Dumazet wrote:
quoted
I think 19bdb70c77d3 should be reverted.
Just reverting the commit will reintroduce the other lockdep WARN that the
commit addressed. I hope to have another fix to avoid the WARN.
I do not think you know the root cause of that lockdep warn, so reverting
19bdb70c77d3 is the right thing to do because of [11, 12].

[11] ffa1e7ada456 ("block: Make request_queue lockdep splats show up earlier")
[12] Subject: Re: [PATCH] nbd: don't warn when reclassifying a busy socket lock
https://lore.kernel.org/lkml/20260816014906.1149-1-hdanton@sina.com/ (local)
I took a look in the commit [11] and the discussion [12], but couldn't follow
why the commit 19bdb70c77d3 should be reverted. Could you elaborate a bit more
about the reason you suggest the revert?
I think you missed one problem in lib/once.c

Revert commit e8eef69a99f1 (or restore once_disable_jump(once_key,
mod) inside __do_once_sleepable_done())
so static_branch_disable() is never called synchronously under caller locks.

Previously, once_disable_jump() deferred static_branch_disable() to a
worker thread via schedule_work().

Commit e8eef69a99f1 made it run synchronously in the caller's context.
Because __inet_hash_connect() is called under lock_sock(sk), calling
static_branch_disable() directly inside DO_ONCE_SLEEPABLE() takes
cpus_read_lock() (cpu_hotplug_lock) while holding lock_sock(sk).

This created the bogus lock dependency sk_lock -> cpu_hotplug_lock.

Every other storage/networking client (sunrpc, nbd, cifs, iscsi_tcp,
rxe, siw) uses static lockdep keys (static struct lock_class_key
...[2]) without issue.
I  sent the revert request for review :
https://lore.kernel.org/lkml/20260825142515.1965654-1-edumazet@google.com/T/#u (local)
Thank you for the clarification. Now I have better understanding.

When I looked at the lock dependency chain among six locks below, I assumed
that any of the dependencies could not be cut.

   set->srcu -> sk_lock -> cpu_hotplug_lock -> fs_reclaim -> q_usage_counter -> elevator_lock -> set->srcu

So I thought that the idea to introduce the dynamic lockdep key for nvme-tcp
sk_lock instances would be the only one solution. But you pointed out that the
dependency sk_lock -> cpu_hotplug_lock can be cut by reverting the commit
e8eef69a99f1.

I did trial and confirmed that your suggestion works. With the kerenl v7.2 +
revert 19bdb70c77d3 + revert e8eef69a99f1, I confirmed the blktests test case
nvme/005 passed with tcp transport. The lockdep WARN due to the six locks goes
away. Great :)

Assuming e8eef69a99f1 will be reverted, I agree to revert 19bdb70c77d3 also.
quoted
Use my pending TCP patch, which is a no brainer :
https://lore.kernel.org/netdev/20260825023614.1228551-1-edumazet@google.com/ (local)
IIUC, this patch uses GFP_ATOMIC to cut the dependency sk_lock -> fs_reclaim.
I did another trial: using the kerenl v7.2 + revert 19bdb70c77d3 + the TCP patch
"tcp: use GFP_ATOMIC in tcp_send_active_reset()", I still observed the blktests
test case nvme/005 failed with the lockdep WARN due to the six locks. Hmm.

The TCP patch cuts the direct dependency sk_lock -> fs_reclaim, but the depdency
chain I face has indirect dependency between sk_lock and fs_reclaim:

  ... sk_lock -> cpu_hotplug_lock -> fs_reclaim ...

I think this explains why the TCP patch does not fix the lockdep WARN due to the
six locks.
quoted
Revert 19bdb70c77d3 in drivers/nvme/host/tcp.c : Restore static
lockdep keys nvme_tcp_sk_key[2] and nvme_tcp_slock_key[2].

And we should be good.

If you think each nvme-tcp socket MUST have its own LOCKDEP class,
please elaborate, because I have no idea
why this would be needed.
As I noted above, if e8eef69a99f1 will be reverted, I do not have reason to
make nvme-tcp socket have its owy dynamic lockdep key and class. Let me wait
and see how the e8eef69a99f1 revert patch will go. If it goes well, I will
send out the patch to revert 19bdb70c77d3.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help