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

[Nilay Shroff]

On 9/14/26 8:28 PM, Eric Dumazet wrote:
> 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.
> 
Makes sense... Thanks for the explanation!

--Nilay