Commit a3ff3ebd authored by Dan Williams's avatar Dan Williams Committed by Greg Kroah-Hartman

libnvdimm: fix nvdimm_bus_lock() vs device_lock() ordering

commit 452bae0a upstream.

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.

Fixes: 8c2f7e86 ("libnvdimm: infrastructure for btt devices")
Reported-by: default avatarYi Zhang <yizhan@redhat.com>
Signed-off-by: default avatarDan Williams <dan.j.williams@intel.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent de21b800
...@@ -314,7 +314,7 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns) ...@@ -314,7 +314,7 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
if (rc < 0) { if (rc < 0) {
struct nd_btt *nd_btt = to_nd_btt(btt_dev); struct nd_btt *nd_btt = to_nd_btt(btt_dev);
__nd_detach_ndns(btt_dev, &nd_btt->ndns); nd_detach_ndns(btt_dev, &nd_btt->ndns);
put_device(btt_dev); put_device(btt_dev);
} }
......
...@@ -21,8 +21,13 @@ ...@@ -21,8 +21,13 @@
void __nd_detach_ndns(struct device *dev, struct nd_namespace_common **_ndns) void __nd_detach_ndns(struct device *dev, struct nd_namespace_common **_ndns)
{ {
struct nd_namespace_common *ndns = *_ndns; struct nd_namespace_common *ndns = *_ndns;
struct nvdimm_bus *nvdimm_bus;
lockdep_assert_held(&ndns->dev.mutex); if (!ndns)
return;
nvdimm_bus = walk_to_nvdimm_bus(&ndns->dev);
lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
dev_WARN_ONCE(dev, ndns->claim != dev, "%s: invalid claim\n", __func__); dev_WARN_ONCE(dev, ndns->claim != dev, "%s: invalid claim\n", __func__);
ndns->claim = NULL; ndns->claim = NULL;
*_ndns = NULL; *_ndns = NULL;
...@@ -37,18 +42,20 @@ void nd_detach_ndns(struct device *dev, ...@@ -37,18 +42,20 @@ void nd_detach_ndns(struct device *dev,
if (!ndns) if (!ndns)
return; return;
get_device(&ndns->dev); get_device(&ndns->dev);
device_lock(&ndns->dev); nvdimm_bus_lock(&ndns->dev);
__nd_detach_ndns(dev, _ndns); __nd_detach_ndns(dev, _ndns);
device_unlock(&ndns->dev); nvdimm_bus_unlock(&ndns->dev);
put_device(&ndns->dev); put_device(&ndns->dev);
} }
bool __nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach, bool __nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach,
struct nd_namespace_common **_ndns) struct nd_namespace_common **_ndns)
{ {
struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(&attach->dev);
if (attach->claim) if (attach->claim)
return false; return false;
lockdep_assert_held(&attach->dev.mutex); lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
dev_WARN_ONCE(dev, *_ndns, "%s: invalid claim\n", __func__); dev_WARN_ONCE(dev, *_ndns, "%s: invalid claim\n", __func__);
attach->claim = dev; attach->claim = dev;
*_ndns = attach; *_ndns = attach;
...@@ -61,9 +68,9 @@ bool nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach, ...@@ -61,9 +68,9 @@ bool nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach,
{ {
bool claimed; bool claimed;
device_lock(&attach->dev); nvdimm_bus_lock(&attach->dev);
claimed = __nd_attach_ndns(dev, attach, _ndns); claimed = __nd_attach_ndns(dev, attach, _ndns);
device_unlock(&attach->dev); nvdimm_bus_unlock(&attach->dev);
return claimed; return claimed;
} }
...@@ -114,7 +121,7 @@ static void nd_detach_and_reset(struct device *dev, ...@@ -114,7 +121,7 @@ static void nd_detach_and_reset(struct device *dev,
struct nd_namespace_common **_ndns) struct nd_namespace_common **_ndns)
{ {
/* detach the namespace and destroy / reset the device */ /* detach the namespace and destroy / reset the device */
nd_detach_ndns(dev, _ndns); __nd_detach_ndns(dev, _ndns);
if (is_idle(dev, *_ndns)) { if (is_idle(dev, *_ndns)) {
nd_device_unregister(dev, ND_ASYNC); nd_device_unregister(dev, ND_ASYNC);
} else if (is_nd_btt(dev)) { } else if (is_nd_btt(dev)) {
...@@ -184,7 +191,7 @@ ssize_t nd_namespace_store(struct device *dev, ...@@ -184,7 +191,7 @@ ssize_t nd_namespace_store(struct device *dev,
} }
WARN_ON_ONCE(!is_nvdimm_bus_locked(dev)); WARN_ON_ONCE(!is_nvdimm_bus_locked(dev));
if (!nd_attach_ndns(dev, ndns, _ndns)) { if (!__nd_attach_ndns(dev, ndns, _ndns)) {
dev_dbg(dev, "%s already claimed\n", dev_dbg(dev, "%s already claimed\n",
dev_name(&ndns->dev)); dev_name(&ndns->dev));
len = -EBUSY; len = -EBUSY;
......
...@@ -124,7 +124,7 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns) ...@@ -124,7 +124,7 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
dev_dbg(dev, "%s: dax: %s\n", __func__, dev_dbg(dev, "%s: dax: %s\n", __func__,
rc == 0 ? dev_name(dax_dev) : "<none>"); rc == 0 ? dev_name(dax_dev) : "<none>");
if (rc < 0) { if (rc < 0) {
__nd_detach_ndns(dax_dev, &nd_pfn->ndns); nd_detach_ndns(dax_dev, &nd_pfn->ndns);
put_device(dax_dev); put_device(dax_dev);
} else } else
__nd_device_register(dax_dev); __nd_device_register(dax_dev);
......
...@@ -484,7 +484,7 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns) ...@@ -484,7 +484,7 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
dev_dbg(dev, "%s: pfn: %s\n", __func__, dev_dbg(dev, "%s: pfn: %s\n", __func__,
rc == 0 ? dev_name(pfn_dev) : "<none>"); rc == 0 ? dev_name(pfn_dev) : "<none>");
if (rc < 0) { if (rc < 0) {
__nd_detach_ndns(pfn_dev, &nd_pfn->ndns); nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
put_device(pfn_dev); put_device(pfn_dev);
} else } else
__nd_device_register(pfn_dev); __nd_device_register(pfn_dev);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment