From: <gregkh@linuxfoundation.org>
To: dan.j.williams@intel.com, gregkh@linuxfoundation.org, yizhan@redhat.com
Cc: <stable@vger.kernel.org>, <stable-commits@vger.kernel.org>
Subject: Patch "device-dax: fix sysfs attribute deadlock" has been added to the 4.11-stable tree
Date: Thu, 18 May 2017 09:45:15 +0200 [thread overview]
Message-ID: <149509351572246@kroah.com> (raw)
This is a note to let you know that I've just added the patch titled
device-dax: fix sysfs attribute deadlock
to the 4.11-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
device-dax-fix-sysfs-attribute-deadlock.patch
and it can be found in the queue-4.11 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From 565851c972b50612f3a4542e26879ffb3e906fc2 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Sun, 30 Apr 2017 06:57:01 -0700
Subject: device-dax: fix sysfs attribute deadlock
From: Dan Williams <dan.j.williams@intel.com>
commit 565851c972b50612f3a4542e26879ffb3e906fc2 upstream.
Usage of device_lock() for dax_region attributes is unnecessary and
deadlock prone. It's unnecessary because the order of registration /
un-registration guarantees that drvdata is always valid. It's deadlock
prone because it sets up this situation:
ndctl D 0 2170 2082 0x00000000
Call Trace:
__schedule+0x31f/0x980
schedule+0x3d/0x90
schedule_preempt_disabled+0x15/0x20
__mutex_lock+0x402/0x980
? __mutex_lock+0x158/0x980
? align_show+0x2b/0x80 [dax]
? kernfs_seq_start+0x2f/0x90
mutex_lock_nested+0x1b/0x20
align_show+0x2b/0x80 [dax]
dev_attr_show+0x20/0x50
ndctl D 0 2186 2079 0x00000000
Call Trace:
__schedule+0x31f/0x980
schedule+0x3d/0x90
__kernfs_remove+0x1f6/0x340
? kernfs_remove_by_name_ns+0x45/0xa0
? remove_wait_queue+0x70/0x70
kernfs_remove_by_name_ns+0x45/0xa0
remove_files.isra.1+0x35/0x70
sysfs_remove_group+0x44/0x90
sysfs_remove_groups+0x2e/0x50
dax_region_unregister+0x25/0x40 [dax]
devm_action_release+0xf/0x20
release_nodes+0x16d/0x2b0
devres_release_all+0x3c/0x60
device_release_driver_internal+0x17d/0x220
device_release_driver+0x12/0x20
unbind_store+0x112/0x160
ndctl/2170 is trying to acquire the device_lock() to read an attribute,
and ndctl/2186 is holding the device_lock() while trying to drain all
active attribute readers.
Thanks to Yi Zhang for the reproduction script.
Fixes: d7fe1a67f658 ("dax: add region 'id', 'size', and 'align' attributes")
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/dax/dax.c | 40 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 28 deletions(-)
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -77,36 +77,27 @@ struct dax_dev {
struct resource res[0];
};
+/*
+ * Rely on the fact that drvdata is set before the attributes are
+ * registered, and that the attributes are unregistered before drvdata
+ * is cleared to assume that drvdata is always valid.
+ */
static ssize_t id_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct dax_region *dax_region;
- ssize_t rc = -ENXIO;
+ struct dax_region *dax_region = dev_get_drvdata(dev);
- device_lock(dev);
- dax_region = dev_get_drvdata(dev);
- if (dax_region)
- rc = sprintf(buf, "%d\n", dax_region->id);
- device_unlock(dev);
-
- return rc;
+ return sprintf(buf, "%d\n", dax_region->id);
}
static DEVICE_ATTR_RO(id);
static ssize_t region_size_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct dax_region *dax_region;
- ssize_t rc = -ENXIO;
-
- device_lock(dev);
- dax_region = dev_get_drvdata(dev);
- if (dax_region)
- rc = sprintf(buf, "%llu\n", (unsigned long long)
- resource_size(&dax_region->res));
- device_unlock(dev);
+ struct dax_region *dax_region = dev_get_drvdata(dev);
- return rc;
+ return sprintf(buf, "%llu\n", (unsigned long long)
+ resource_size(&dax_region->res));
}
static struct device_attribute dev_attr_region_size = __ATTR(size, 0444,
region_size_show, NULL);
@@ -114,16 +105,9 @@ static struct device_attribute dev_attr_
static ssize_t align_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct dax_region *dax_region;
- ssize_t rc = -ENXIO;
-
- device_lock(dev);
- dax_region = dev_get_drvdata(dev);
- if (dax_region)
- rc = sprintf(buf, "%u\n", dax_region->align);
- device_unlock(dev);
+ struct dax_region *dax_region = dev_get_drvdata(dev);
- return rc;
+ return sprintf(buf, "%u\n", dax_region->align);
}
static DEVICE_ATTR_RO(align);
Patches currently in stable-queue which might be from dan.j.williams@intel.com are
queue-4.11/ext4-return-to-starting-transaction-in-ext4_dax_huge_fault.patch
queue-4.11/x86-pmem-fix-cache-flushing-for-iovec-write-8-bytes.patch
queue-4.11/dax-prevent-invalidation-of-mapped-dax-entries.patch
queue-4.11/dax-fix-pmd-data-corruption-when-fault-races-with-write.patch
queue-4.11/device-dax-fix-cdev-leak.patch
queue-4.11/device-dax-fix-sysfs-attribute-deadlock.patch
queue-4.11/mm-fix-data-corruption-due-to-stale-mmap-reads.patch
reply other threads:[~2017-05-18 7:45 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=149509351572246@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=dan.j.williams@intel.com \
--cc=stable-commits@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=yizhan@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.