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

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Aug 25, 2026 / 16:27, Eric Dumazet wrote:
> On Tue, Aug 25, 2026 at 3:53 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Aug 25, 2026 at 3:26 PM Shin'ichiro Kawasaki
> > <shinichiro.kawasaki@wdc.com> wrote:
> > >
> > > On Aug 25, 2026 / 14:45, Hillf Danton wrote:
> > > > On Tue, 25 Aug 2026 14:44:35 +0900 Shin'ichiro Kawasaki wrote:
> > > > > On Aug 25, 2026 / 03:50, Eric Dumazet wrote:
> > > > > >
> > > > > > 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/
> > >
> > > 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

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.

> 
> >
> > Use my pending TCP patch, which is a no brainer :
> > https://lore.kernel.org/netdev/20260825023614.1228551-1-edumazet@google.com/

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.

> >
> > 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.