[PATCH 0/2] nvme-tcp: do not use dynamic lockdep key for socket instances

[Eric Dumazet]

On Mon, Sep 14, 2026 at 7:48 AM Nilay Shroff <nilay at linux.ibm.com> wrote:
>
> On 9/14/26 1:18 PM, Shin'ichiro Kawasaki wrote:
> > From: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>
> >
> > Keith and the maintainers, please consider this series for upstream.
> > The commit 19bdb70c77d3 tried to avoid a lockdep WARN issue, but it
> > was imperfect. With Eric's help, I propose this series as a better
> > solution.
> >
> > Eric, FYI, I prepared the second patch with your authorship and your
> > SoB tag. Thanks for the fix idea.
> >
> >
> > Commit 19bdb70c77d3 ("nvme-tcp: lockdep: use dynamic lockdep keys per
> > socket instance") introduced the dynamic lockdep key to avoid a lockdep
> > WARN. However, it had a bug in lockdep key lifetime management and
> > caused another WARN [1]. The first patch in this series reverts the
> > commit to avoid the WARN.
> >
> > Reverting 19bdb70c77d3 re-exposes two lockdep WARNs that it had
> > suppressed. The first one was observed with the blktests test case
> > nvme/005, which was caused by the lock chain below:
> >
> >    set->srcu -> sk_lock -> cpu_hotplug_lock -> fs_reclaim -> q_usage_counter -> elevator_lock -> set->srcu
> >
> > This WARN needs no patch in this series: it is already cut by the merged
> > commit 0ba6912f7e97 by Eric in the v7.3-rc2 tag, which removes the
> > "sk_lock -> cpu_hotplug_lock" dependency. This series therefore depends
> > on that commit being present.
> >
> > The second WARN was observed with the blktests test case nvme/062, which
> > was caused by the dependency newly added for TLS support. The second
> > patch in this series delays the socket reclassification timing to cut
> > the dependency.
> >
> > With this series applied on v7.3-rc2, nvme/005 and nvme/062 pass with no
> > lockdep splat.
> >
> I have just reviewed both the patch in the series and both looks good to me.
>
> I also looked at Eric's commits 18666c73afe9 ("tcp: use GFP_ATOMIC in
> tcp_send_active_reset()") and 0ba6912f7e97 ("Revert once: don't use a work
> queue to reset sleepable static key"). Both look good to me.
>
> However, reverting commit 19bdb70c77d3 ("nvme-tcp: lockdep: use dynamic lockdep
> keys per socket instance") means that lockdep will no longer be able to distinguish
> between the socket locks of different nvme-tcp queues. This could unnecessarily
> create dependency chains between socket locks belonging to different queues,
> even though those sockets are independent in practice.

This is not a problem, and there is no need for a per-flow lockdep class.

Lockdep tracks lock _classes_, not lock instances, by design. All TCP
sockets in the kernel share "slock-AF_INET"/"sk_lock-AF_INET", and a busy
machine has millions of them. If lockdep needed a class per socket to
produce useful reports, networking would have been unusable with
CONFIG_LOCKDEP=y for the last twenty years.

Per-instance keys are only warranted when two locks of the same class are
held at the same time, in a well-defined order. nvme-tcp never holds one
queue's sk_lock while acquiring another queue's sk_lock, so there is
nothing to disambiguate. And if such a nesting were ever introduced, the
right annotation is a lockdep subclass (nested annotation), not a
dynamically registered key per object.

Note also that per-instance keys do not make lockdep smarter, they make it
blind: they remove the ability to detect a real AB/BA between two nvme-tcp
sockets. Silencing a class of reports before any false positive has been
demonstrated is the wrong trade.

The only distinction that has ever been needed here is "in-kernel nvme-tcp
socket" vs "user socket", which is exactly what 841aee4d75f1 ("nvme-tcp:
lockdep: annotate in-kernel sockets") provides with static keys. Patch 1
restores that, and patch 2 moves the reclassification after the TLS
handshake so tlshd's dependencies are not pulled in.

>
> So, while Eric's two commits above should address the lockdep splat reported by
> blktests nvme/005 (as well as syzbot reported warning), IMO we should still teach
> lockdep that sockets belonging to different nvme-tcp queues have different lock
> classes. This would prevent lockdep from constructing dependency chains between
> unrelated socket instances.
>
> I agree with Eric's initial assessment that the lifetime of the nvme-tcp queue and
> the lifetime of the lockdep key associated with its socket need to be tracked separately.
> Based on that, perhaps we should consider introducing a separate object to track the
> socket lockdep key lifetime (separate from nvme-tcp queue object), similar to what
> Shin'ichiro proposed here:
> https://lore.kernel.org/lkml/ao2QHIDrGeYjltX9@shinhome/

I would rather not. My point back then was that 19bdb70c77d3 was buggy
because sockets outlive the nvme-tcp queue: __fput_sync() does not
guarantee sk destruction (in-flight packets, TIME_WAIT, RCU-deferred
sk_free()). The conclusion I draw from that is "do not tie a lockdep key to
this lifetime at all", not "add a refcounted object whose only purpose is
to hold a debug-only key".

Such an object would add allocation, refcounting and teardown paths in
nvme-tcp that exist only for CONFIG_DEBUG_LOCK_ALLOC=y builds, i.e. code
that 99% of users never execute and that nobody will keep correct. We
already have one bug from exactly this, plus the syzbot report. Adding
more machinery around it is how we get the third one.

It is also not free at runtime: lockdep has a finite key space
(MAX_LOCKDEP_KEYS), and lockdep_register_key()/lockdep_unregister_key() are
heavy operations (graph lock, chain hash zapping, RCU). nvme-tcp creates
one queue per CPU per controller and re-creates all of them on every
reconnect. Churning two keys per queue per reconnect is not something we
should do for a debug feature.

No other kernel socket user (sunrpc, ceph, rds, iscsi, drbd, ...) does
this. nvme-tcp should not be the exception.

Thanks.

>
> Any thoughts?
>
> Thanks,
> --Nilay