Skip to content

Commit 452bae0

Browse files
committed
libnvdimm: fix nvdimm_bus_lock() vs device_lock() ordering
A debug patch to turn the standard device_lock() into something that lockdep can analyze yielded the following: ====================================================== [ INFO: possible circular locking dependency detected ] 4.11.0-rc4+ #106 Tainted: G O ------------------------------------------------------- lt-libndctl/1898 is trying to acquire lock: (&dev->nvdimm_mutex/3){+.+.+.}, at: [<ffffffffc023c948>] nd_attach_ndns+0x178/0x1b0 [libnvdimm] but task is already holding lock: (&nvdimm_bus->reconfig_mutex){+.+.+.}, at: [<ffffffffc022e0b1>] nvdimm_bus_lock+0x21/0x30 [libnvdimm] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&nvdimm_bus->reconfig_mutex){+.+.+.}: lock_acquire+0xf6/0x1f0 __mutex_lock+0x88/0x980 mutex_lock_nested+0x1b/0x20 nvdimm_bus_lock+0x21/0x30 [libnvdimm] nvdimm_namespace_capacity+0x1b/0x40 [libnvdimm] nvdimm_namespace_common_probe+0x230/0x510 [libnvdimm] nd_pmem_probe+0x14/0x180 [nd_pmem] nvdimm_bus_probe+0xa9/0x260 [libnvdimm] -> #0 (&dev->nvdimm_mutex/3){+.+.+.}: __lock_acquire+0x1107/0x1280 lock_acquire+0xf6/0x1f0 __mutex_lock+0x88/0x980 mutex_lock_nested+0x1b/0x20 nd_attach_ndns+0x178/0x1b0 [libnvdimm] nd_namespace_store+0x308/0x3c0 [libnvdimm] namespace_store+0x87/0x220 [libnvdimm] In this case '&dev->nvdimm_mutex/3' mirrors '&dev->mutex'. Fix this by replacing the use of device_lock() with nvdimm_bus_lock() to protect nd_{attach,detach}_ndns() operations. Cc: <[email protected]> Fixes: 8c2f7e8 ("libnvdimm: infrastructure for btt devices") Reported-by: Yi Zhang <[email protected]> Signed-off-by: Dan Williams <[email protected]>
1 parent 23f4984 commit 452bae0

File tree

4 files changed

+18
-11
lines changed

4 files changed

+18
-11
lines changed

drivers/nvdimm/btt_devs.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
314314
if (rc < 0) {
315315
struct nd_btt *nd_btt = to_nd_btt(btt_dev);
316316

317-
__nd_detach_ndns(btt_dev, &nd_btt->ndns);
317+
nd_detach_ndns(btt_dev, &nd_btt->ndns);
318318
put_device(btt_dev);
319319
}
320320

drivers/nvdimm/claim.c

+15-8
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,13 @@
2121
void __nd_detach_ndns(struct device *dev, struct nd_namespace_common **_ndns)
2222
{
2323
struct nd_namespace_common *ndns = *_ndns;
24+
struct nvdimm_bus *nvdimm_bus;
2425

25-
lockdep_assert_held(&ndns->dev.mutex);
26+
if (!ndns)
27+
return;
28+
29+
nvdimm_bus = walk_to_nvdimm_bus(&ndns->dev);
30+
lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
2631
dev_WARN_ONCE(dev, ndns->claim != dev, "%s: invalid claim\n", __func__);
2732
ndns->claim = NULL;
2833
*_ndns = NULL;
@@ -37,18 +42,20 @@ void nd_detach_ndns(struct device *dev,
3742
if (!ndns)
3843
return;
3944
get_device(&ndns->dev);
40-
device_lock(&ndns->dev);
45+
nvdimm_bus_lock(&ndns->dev);
4146
__nd_detach_ndns(dev, _ndns);
42-
device_unlock(&ndns->dev);
47+
nvdimm_bus_unlock(&ndns->dev);
4348
put_device(&ndns->dev);
4449
}
4550

4651
bool __nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach,
4752
struct nd_namespace_common **_ndns)
4853
{
54+
struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(&attach->dev);
55+
4956
if (attach->claim)
5057
return false;
51-
lockdep_assert_held(&attach->dev.mutex);
58+
lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
5259
dev_WARN_ONCE(dev, *_ndns, "%s: invalid claim\n", __func__);
5360
attach->claim = dev;
5461
*_ndns = attach;
@@ -61,9 +68,9 @@ bool nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach,
6168
{
6269
bool claimed;
6370

64-
device_lock(&attach->dev);
71+
nvdimm_bus_lock(&attach->dev);
6572
claimed = __nd_attach_ndns(dev, attach, _ndns);
66-
device_unlock(&attach->dev);
73+
nvdimm_bus_unlock(&attach->dev);
6774
return claimed;
6875
}
6976

@@ -114,7 +121,7 @@ static void nd_detach_and_reset(struct device *dev,
114121
struct nd_namespace_common **_ndns)
115122
{
116123
/* detach the namespace and destroy / reset the device */
117-
nd_detach_ndns(dev, _ndns);
124+
__nd_detach_ndns(dev, _ndns);
118125
if (is_idle(dev, *_ndns)) {
119126
nd_device_unregister(dev, ND_ASYNC);
120127
} else if (is_nd_btt(dev)) {
@@ -184,7 +191,7 @@ ssize_t nd_namespace_store(struct device *dev,
184191
}
185192

186193
WARN_ON_ONCE(!is_nvdimm_bus_locked(dev));
187-
if (!nd_attach_ndns(dev, ndns, _ndns)) {
194+
if (!__nd_attach_ndns(dev, ndns, _ndns)) {
188195
dev_dbg(dev, "%s already claimed\n",
189196
dev_name(&ndns->dev));
190197
len = -EBUSY;

drivers/nvdimm/dax_devs.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
124124
dev_dbg(dev, "%s: dax: %s\n", __func__,
125125
rc == 0 ? dev_name(dax_dev) : "<none>");
126126
if (rc < 0) {
127-
__nd_detach_ndns(dax_dev, &nd_pfn->ndns);
127+
nd_detach_ndns(dax_dev, &nd_pfn->ndns);
128128
put_device(dax_dev);
129129
} else
130130
__nd_device_register(dax_dev);

drivers/nvdimm/pfn_devs.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
484484
dev_dbg(dev, "%s: pfn: %s\n", __func__,
485485
rc == 0 ? dev_name(pfn_dev) : "<none>");
486486
if (rc < 0) {
487-
__nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
487+
nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
488488
put_device(pfn_dev);
489489
} else
490490
__nd_device_register(pfn_dev);

0 commit comments

Comments
 (0)