* [PATCH 1/2] mm, devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin, done}
2017-02-12 22:34 [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Dan Williams
@ 2017-02-12 22:34 ` Dan Williams
2017-02-12 22:34 ` [PATCH 2/2] mm: validate device_hotplug is held for memory hotplug Dan Williams
2017-02-13 1:25 ` [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Masayoshi Mizuma
2 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2017-02-12 22:34 UTC (permalink / raw)
To: akpm
Cc: Michal Hocko, Toshi Kani, linux-nvdimm, Logan Gunthorpe,
linux-kernel, stable, Ben Hutchings, Vlastimil Babka
The mem_hotplug_{begin,done} lock coordinates with
{get,put}_online_mems() to hold off "readers" of the current state of
memory from new hotplug actions. mem_hotplug_begin() expects exclusive
access, via the device_hotplug lock, to set mem_hotplug.active_writer.
Calling mem_hotplug_begin() without locking device_hotplug can lead to
corrupting mem_hotplug.refcount and missed wakeups / soft lockups.
Cc: <stable@vger.kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Logan Gunthorpe <logang@deltatee.com>
Fixes: f931ab479dd2 ("mm: fix devm_memremap_pages crash, use mem_hotplug_{begin, done}")
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
kernel/memremap.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index e6476a8e8b6a..16640415c150 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -264,9 +264,12 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
}
/* pages are dead and unused, undo the arch mapping */
+ lock_device_hotplug();
mem_hotplug_begin();
arch_remove_memory(res->start, resource_size(res));
mem_hotplug_done();
+ unlock_device_hotplug();
+
untrack_pfn(NULL, PHYS_PFN(res->start), resource_size(res));
pgmap_radix_release(res);
dev_WARN_ONCE(dev, pgmap->altmap && pgmap->altmap->alloc,
@@ -374,9 +377,11 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
if (error)
goto err_pfn_remap;
+ lock_device_hotplug();
mem_hotplug_begin();
error = arch_add_memory(nid, res->start, resource_size(res), true);
mem_hotplug_done();
+ unlock_device_hotplug();
if (error)
goto err_add_memory;
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/2] mm: validate device_hotplug is held for memory hotplug
2017-02-12 22:34 [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Dan Williams
2017-02-12 22:34 ` [PATCH 1/2] mm, devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin, done} Dan Williams
@ 2017-02-12 22:34 ` Dan Williams
2017-02-13 1:25 ` [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Masayoshi Mizuma
2 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2017-02-12 22:34 UTC (permalink / raw)
To: akpm
Cc: Michal Hocko, Toshi Kani, Ben Hutchings, Greg Kroah-Hartman,
linux-nvdimm, linux-kernel, Logan Gunthorpe, Vlastimil Babka
mem_hotplug_begin() assumes that it can set mem_hotplug.active_writer
and run the hotplug process without racing another thread. Validate this
assumption with a lockdep assertion.
Cc: Michal Hocko <mhocko@suse.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/base/core.c | 5 +++++
include/linux/device.h | 1 +
mm/memory_hotplug.c | 2 ++
3 files changed, 8 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8c25e68e67d7..3050e6f99403 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -638,6 +638,11 @@ int lock_device_hotplug_sysfs(void)
return restart_syscall();
}
+void assert_held_device_hotplug(void)
+{
+ lockdep_assert_held(&device_hotplug_lock);
+}
+
#ifdef CONFIG_BLOCK
static inline int device_is_not_partition(struct device *dev)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index bd684fc8ec1d..a48a7ff70164 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1139,6 +1139,7 @@ static inline bool device_supports_offline(struct device *dev)
extern void lock_device_hotplug(void);
extern void unlock_device_hotplug(void);
extern int lock_device_hotplug_sysfs(void);
+void assert_held_device_hotplug(void);
extern int device_offline(struct device *dev);
extern int device_online(struct device *dev);
extern void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 19b460acb5e1..0ab3dd6cc2a4 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -126,6 +126,8 @@ void put_online_mems(void)
void mem_hotplug_begin(void)
{
+ assert_held_device_hotplug();
+
mem_hotplug.active_writer = current;
memhp_lock_acquire();
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking
2017-02-12 22:34 [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Dan Williams
2017-02-12 22:34 ` [PATCH 1/2] mm, devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin, done} Dan Williams
2017-02-12 22:34 ` [PATCH 2/2] mm: validate device_hotplug is held for memory hotplug Dan Williams
@ 2017-02-13 1:25 ` Masayoshi Mizuma
2017-02-13 1:35 ` Dan Williams
2 siblings, 1 reply; 6+ messages in thread
From: Masayoshi Mizuma @ 2017-02-13 1:25 UTC (permalink / raw)
To: Dan Williams, akpm
Cc: Michal Hocko, linux-nvdimm, Greg Kroah-Hartman, linux-kernel,
stable, Ben Hutchings, Vlastimil Babka
Hi Dan,
On Sun, 12 Feb 2017 14:34:11 -0800 Dan Williams wrote:
> Ben notes that commit f931ab479dd2 "mm: fix devm_memremap_pages crash,
> use mem_hotplug_{begin, done}" is incomplete and broken. Writes to
> mem_hotplug.active_writer need to be coordinated under the device
> hotplug lock. Otherwise, we can potentially corrupt mem_hotplug.refcount
> leading to soft lockups.
I think mem_hotplug_{begin,done} is not suitable to exclude devm_memremap_pages()
because it seems that memory hotplug is not related to this context.
How about using pgmap_lock instead?
Like this:
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 9ecedc2..e9b9cfa 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -246,9 +246,9 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
/* pages are dead and unused, undo the arch mapping */
align_start = res->start & ~(SECTION_SIZE - 1);
align_size = ALIGN(resource_size(res), SECTION_SIZE);
- mem_hotplug_begin();
+ mutex_lock(&pgmap_lock);
arch_remove_memory(align_start, align_size);
- mem_hotplug_done();
+ mutex_unlock(&pgmap_lock);
untrack_pfn(NULL, PHYS_PFN(align_start), align_size);
pgmap_radix_release(res);
dev_WARN_ONCE(dev, pgmap->altmap && pgmap->altmap->alloc,
@@ -360,9 +360,9 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
if (error)
goto err_pfn_remap;
- mem_hotplug_begin();
+ mutex_lock(&pgmap_lock);
error = arch_add_memory(nid, align_start, align_size, true);
- mem_hotplug_done();
+ mutex_unlock(&pgmap_lock);
if (error)
goto err_add_memory;
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking
2017-02-13 1:25 ` [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking Masayoshi Mizuma
@ 2017-02-13 1:35 ` Dan Williams
2017-02-15 0:24 ` Masayoshi Mizuma
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2017-02-13 1:35 UTC (permalink / raw)
To: Masayoshi Mizuma
Cc: Andrew Morton, Michal Hocko, linux-nvdimm@lists.01.org,
Greg Kroah-Hartman, linux-kernel, stable, Ben Hutchings,
Vlastimil Babka
On Sun, Feb 12, 2017 at 5:25 PM, Masayoshi Mizuma
<m.mizuma@jp.fujitsu.com> wrote:
> Hi Dan,
>
> On Sun, 12 Feb 2017 14:34:11 -0800 Dan Williams wrote:
>>
>> Ben notes that commit f931ab479dd2 "mm: fix devm_memremap_pages crash,
>> use mem_hotplug_{begin, done}" is incomplete and broken. Writes to
>> mem_hotplug.active_writer need to be coordinated under the device
>> hotplug lock. Otherwise, we can potentially corrupt mem_hotplug.refcount
>> leading to soft lockups.
>
>
> I think mem_hotplug_{begin,done} is not suitable to exclude
> devm_memremap_pages()
> because it seems that memory hotplug is not related to this context.
> How about using pgmap_lock instead?
The problem with that switch is the race in
kernel_physical_mapping_init(), see commit f931ab479dd2. As far as I
can see we need all paths that call kernel_physical_mapping_init() to
agree on the same lock, and can't use a private lock in
devm_memremap_pages().
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking
2017-02-13 1:35 ` Dan Williams
@ 2017-02-15 0:24 ` Masayoshi Mizuma
0 siblings, 0 replies; 6+ messages in thread
From: Masayoshi Mizuma @ 2017-02-15 0:24 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Michal Hocko, linux-nvdimm@lists.01.org,
Greg Kroah-Hartman, linux-kernel, stable, Ben Hutchings,
Vlastimil Babka
On Sun, 12 Feb 2017 17:35:00 -0800 Dan Williams wrote:
> On Sun, Feb 12, 2017 at 5:25 PM, Masayoshi Mizuma
> <m.mizuma@jp.fujitsu.com> wrote:
> > Hi Dan,
> >
> > On Sun, 12 Feb 2017 14:34:11 -0800 Dan Williams wrote:
> >>
> >> Ben notes that commit f931ab479dd2 "mm: fix devm_memremap_pages crash,
> >> use mem_hotplug_{begin, done}" is incomplete and broken. Writes to
> >> mem_hotplug.active_writer need to be coordinated under the device
> >> hotplug lock. Otherwise, we can potentially corrupt mem_hotplug.refcount
> >> leading to soft lockups.
> >
> >
> > I think mem_hotplug_{begin,done} is not suitable to exclude
> > devm_memremap_pages()
> > because it seems that memory hotplug is not related to this context.
> > How about using pgmap_lock instead?
>
> The problem with that switch is the race in
> kernel_physical_mapping_init(), see commit f931ab479dd2. As far as I
> can see we need all paths that call kernel_physical_mapping_init() to
> agree on the same lock, and can't use a private lock in
> devm_memremap_pages().
I understand. Thanks.
---
Masayoshi Mizuma
^ permalink raw reply [flat|nested] 6+ messages in thread