| From 452bae0aede774f87bf56c28b6dd50b72c78986c Mon Sep 17 00:00:00 2001 |
| From: Dan Williams <dan.j.williams@intel.com> |
| Date: Fri, 28 Apr 2017 22:05:14 -0700 |
| Subject: libnvdimm: fix nvdimm_bus_lock() vs device_lock() ordering |
| |
| From: Dan Williams <dan.j.williams@intel.com> |
| |
| commit 452bae0aede774f87bf56c28b6dd50b72c78986c 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: 8c2f7e8658df ("libnvdimm: infrastructure for btt devices") |
| Reported-by: Yi Zhang <yizhan@redhat.com> |
| Signed-off-by: Dan Williams <dan.j.williams@intel.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/nvdimm/btt_devs.c | 2 +- |
| drivers/nvdimm/claim.c | 23 +++++++++++++++-------- |
| drivers/nvdimm/dax_devs.c | 2 +- |
| drivers/nvdimm/pfn_devs.c | 2 +- |
| 4 files changed, 18 insertions(+), 11 deletions(-) |
| |
| --- a/drivers/nvdimm/btt_devs.c |
| +++ b/drivers/nvdimm/btt_devs.c |
| @@ -314,7 +314,7 @@ int nd_btt_probe(struct device *dev, str |
| if (rc < 0) { |
| 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); |
| } |
| |
| --- a/drivers/nvdimm/claim.c |
| +++ b/drivers/nvdimm/claim.c |
| @@ -21,8 +21,13 @@ |
| void __nd_detach_ndns(struct device *dev, struct nd_namespace_common **_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__); |
| ndns->claim = NULL; |
| *_ndns = NULL; |
| @@ -37,18 +42,20 @@ void nd_detach_ndns(struct device *dev, |
| if (!ndns) |
| return; |
| get_device(&ndns->dev); |
| - device_lock(&ndns->dev); |
| + nvdimm_bus_lock(&ndns->dev); |
| __nd_detach_ndns(dev, _ndns); |
| - device_unlock(&ndns->dev); |
| + nvdimm_bus_unlock(&ndns->dev); |
| put_device(&ndns->dev); |
| } |
| |
| bool __nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach, |
| struct nd_namespace_common **_ndns) |
| { |
| + struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(&attach->dev); |
| + |
| if (attach->claim) |
| 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__); |
| attach->claim = dev; |
| *_ndns = attach; |
| @@ -61,9 +68,9 @@ bool nd_attach_ndns(struct device *dev, |
| { |
| bool claimed; |
| |
| - device_lock(&attach->dev); |
| + nvdimm_bus_lock(&attach->dev); |
| claimed = __nd_attach_ndns(dev, attach, _ndns); |
| - device_unlock(&attach->dev); |
| + nvdimm_bus_unlock(&attach->dev); |
| return claimed; |
| } |
| |
| @@ -114,7 +121,7 @@ static void nd_detach_and_reset(struct d |
| struct nd_namespace_common **_ndns) |
| { |
| /* detach the namespace and destroy / reset the device */ |
| - nd_detach_ndns(dev, _ndns); |
| + __nd_detach_ndns(dev, _ndns); |
| if (is_idle(dev, *_ndns)) { |
| nd_device_unregister(dev, ND_ASYNC); |
| } else if (is_nd_btt(dev)) { |
| @@ -184,7 +191,7 @@ ssize_t nd_namespace_store(struct device |
| } |
| |
| 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_name(&ndns->dev)); |
| len = -EBUSY; |
| --- a/drivers/nvdimm/dax_devs.c |
| +++ b/drivers/nvdimm/dax_devs.c |
| @@ -124,7 +124,7 @@ int nd_dax_probe(struct device *dev, str |
| dev_dbg(dev, "%s: dax: %s\n", __func__, |
| rc == 0 ? dev_name(dax_dev) : "<none>"); |
| if (rc < 0) { |
| - __nd_detach_ndns(dax_dev, &nd_pfn->ndns); |
| + nd_detach_ndns(dax_dev, &nd_pfn->ndns); |
| put_device(dax_dev); |
| } else |
| __nd_device_register(dax_dev); |
| --- a/drivers/nvdimm/pfn_devs.c |
| +++ b/drivers/nvdimm/pfn_devs.c |
| @@ -484,7 +484,7 @@ int nd_pfn_probe(struct device *dev, str |
| dev_dbg(dev, "%s: pfn: %s\n", __func__, |
| rc == 0 ? dev_name(pfn_dev) : "<none>"); |
| if (rc < 0) { |
| - __nd_detach_ndns(pfn_dev, &nd_pfn->ndns); |
| + nd_detach_ndns(pfn_dev, &nd_pfn->ndns); |
| put_device(pfn_dev); |
| } else |
| __nd_device_register(pfn_dev); |