From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <58A10B25.6030508@jp.fujitsu.com> Date: Mon, 13 Feb 2017 10:25:57 +0900 From: Masayoshi Mizuma MIME-Version: 1.0 Subject: Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking References: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> In-Reply-To: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams , akpm@linux-foundation.org Cc: Michal Hocko , linux-nvdimm@lists.01.org, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Ben Hutchings , Vlastimil Babka List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751483AbdBMBjK (ORCPT ); Sun, 12 Feb 2017 20:39:10 -0500 Received: from mgwym02.jp.fujitsu.com ([211.128.242.41]:27968 "EHLO mgwym02.jp.fujitsu.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbdBMBjI (ORCPT ); Sun, 12 Feb 2017 20:39:08 -0500 X-Greylist: delayed 675 seconds by postgrey-1.27 at vger.kernel.org; Sun, 12 Feb 2017 20:39:07 EST Message-ID: <58A10B25.6030508@jp.fujitsu.com> Date: Mon, 13 Feb 2017 10:25:57 +0900 From: Masayoshi Mizuma User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Dan Williams , CC: Michal Hocko , , Greg Kroah-Hartman , , , Ben Hutchings , Vlastimil Babka Subject: Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking References: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> In-Reply-To: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-SecurityPolicyCheck-GC: OK by FENCE-Mail X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgwym02.jp.fujitsu.com ([211.128.242.41]:27968 "EHLO mgwym02.jp.fujitsu.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbdBMBjI (ORCPT ); Sun, 12 Feb 2017 20:39:08 -0500 Message-ID: <58A10B25.6030508@jp.fujitsu.com> Date: Mon, 13 Feb 2017 10:25:57 +0900 From: Masayoshi Mizuma MIME-Version: 1.0 To: Dan Williams , CC: Michal Hocko , , Greg Kroah-Hartman , , , Ben Hutchings , Vlastimil Babka Subject: Re: [PATCH 0/2] fix devm_memremap_pages() mem hotplug locking References: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> In-Reply-To: <148693885124.16345.14319993365448440148.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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