[PATCH 0/2] fix devm_memremap_pages() mem hotplug locking

[PATCH 0/2] fix devm_memremap_pages() mem hotplug locking

[Dan Williams]

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.

---

Dan Williams (2):
      mm, devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,done}
      mm: validate device_hotplug is held for memory hotplug


 drivers/base/core.c    |    5 +++++
 include/linux/device.h |    1 +
 kernel/memremap.c      |    5 +++++
 mm/memory_hotplug.c    |    2 ++
 4 files changed, 13 insertions(+)

[PATCH 1/2] mm, devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin, done}

[Dan Williams]

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;
 

[PATCH 2/2] mm: validate device_hotplug is held for memory hotplug

[Dan Williams]

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();

Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking

[Masayoshi Mizuma]

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

Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking

[Dan Williams]

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().

Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking

[Masayoshi Mizuma]

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