[PATCH v4 0/5] mm: Fix pfn_to_online_page() with respect to ZONE_DEVICE

[PATCH v4 0/5] mm: Fix pfn_to_online_page() with respect to ZONE_DEVICE

[Dan Williams]

Changes since v3 [1]:
- Switch to "if (IS_ENABLED(CONFIG_HAVE_ARCH_PFN_VALID) &&
  !pfn_valid(pfn))" (David)
- Finish collecting reviewed-bys across all patches in the series
- Drop the libnvdimm fixup, to be merged through nvdimm.git not -mm

[1]: http://lore.kernel.org/r/161052331545.1805594.2356512831689786960.stgit@dwillia2-desk3.amr.corp.intel.com

---

Andrew,

All patches in this series have been reviewed and the kbuild-robot
reports a build-success over 172 configs. They pass an updated version
of the nvdimm unit tests to exercise corner cases of
pfn_to_online_page() and get_dev_pagemap() [2], and apply cleanly to
current -next.

Please apply, thanks.

[2]: http://lore.kernel.org/r/161052209289.1804207.11599120961607513911.stgit@dwillia2-desk3.amr.corp.intel.com

---

Michal reminds that the discussion about how to ensure pfn-walkers do
not get confused by ZONE_DEVICE pages never resolved. A pfn-walker that
uses pfn_to_online_page() may inadvertently translate a pfn as online
and in the page allocator, when it is offline managed by a ZONE_DEVICE
mapping (details in Patch 3: ("mm: Teach pfn_to_online_page() about
ZONE_DEVICE section collisions")).

The 2 proposals under consideration are teach pfn_to_online_page() to be
precise in the presence of mixed-zone sections, or teach the memory-add
code to drop the System RAM associated with ZONE_DEVICE collisions. In
order to not regress memory capacity by a few 10s to 100s of MiB the
approach taken in this set is to add precision to pfn_to_online_page().

In the course of validating pfn_to_online_page() a couple other fixes
fell out:

1/ soft_offline_page() fails to drop the reference taken in the
   madvise(..., MADV_SOFT_OFFLINE) case.

2/ memory_failure() uses get_dev_pagemap() to lookup ZONE_DEVICE pages,
   however that mapping may contain data pages and metadata raw pfns.
   Introduce pgmap_pfn_valid() to delineate the 2 types and fail the
   handling of raw metadata pfns.

---

Dan Williams (5):
      mm: Move pfn_to_online_page() out of line
      mm: Teach pfn_to_online_page() to consider subsection validity
      mm: Teach pfn_to_online_page() about ZONE_DEVICE section collisions
      mm: Fix page reference leak in soft_offline_page()
      mm: Fix memory_failure() handling of dax-namespace metadata


 include/linux/memory_hotplug.h |   17 +---------
 include/linux/memremap.h       |    6 +++
 include/linux/mmzone.h         |   22 +++++++++----
 mm/memory-failure.c            |   26 +++++++++++++--
 mm/memory_hotplug.c            |   69 ++++++++++++++++++++++++++++++++++++++++
 mm/memremap.c                  |   15 +++++++++
 6 files changed, 128 insertions(+), 27 deletions(-)

[PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[Dan Williams]

The conversion to move pfn_to_online_page() internal to
soft_offline_page() missed that the get_user_pages() reference taken by
the madvise() path needs to be dropped when pfn_to_online_page() fails.
Note the direct sysfs-path to soft_offline_page() does not perform a
get_user_pages() lookup.

When soft_offline_page() is handed a pfn_valid() &&
!pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
a leaked reference.

Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
Cc: Michal Hocko <mhocko@kernel.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 mm/memory-failure.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 5a38e9eade94..78b173c7190c 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1885,6 +1885,12 @@ static int soft_offline_free_page(struct page *page)
 	return rc;
 }
 
+static void put_ref_page(struct page *page)
+{
+	if (page)
+		put_page(page);
+}
+
 /**
  * soft_offline_page - Soft offline a page.
  * @pfn: pfn to soft-offline
@@ -1910,20 +1916,26 @@ static int soft_offline_free_page(struct page *page)
 int soft_offline_page(unsigned long pfn, int flags)
 {
 	int ret;
-	struct page *page;
 	bool try_again = true;
+	struct page *page, *ref_page = NULL;
+
+	WARN_ON_ONCE(!pfn_valid(pfn) && (flags & MF_COUNT_INCREASED));
 
 	if (!pfn_valid(pfn))
 		return -ENXIO;
+	if (flags & MF_COUNT_INCREASED)
+		ref_page = pfn_to_page(pfn);
+
 	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
 	page = pfn_to_online_page(pfn);
-	if (!page)
+	if (!page) {
+		put_ref_page(ref_page);
 		return -EIO;
+	}
 
 	if (PageHWPoison(page)) {
 		pr_info("%s: %#lx page already poisoned\n", __func__, pfn);
-		if (flags & MF_COUNT_INCREASED)
-			put_page(page);
+		put_ref_page(ref_page);
 		return 0;
 	}
 

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[HORIGUCHI NAOYA(堀口 直也)]

On Wed, Jan 13, 2021 at 04:43:32PM -0800, Dan Williams wrote:
> The conversion to move pfn_to_online_page() internal to
> soft_offline_page() missed that the get_user_pages() reference taken by
> the madvise() path needs to be dropped when pfn_to_online_page() fails.
> Note the direct sysfs-path to soft_offline_page() does not perform a
> get_user_pages() lookup.
> 
> When soft_offline_page() is handed a pfn_valid() &&
> !pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
> a leaked reference.
> 
> Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm OK if we don't have any other better approach, but the proposed changes
make code a little messy, and I feel that get_user_pages() might be the
right place to fix. Is get_user_pages() expected to return struct page with
holding refcount for offline valid pages?  I thought that such pages are
only used by drivers for dax-devices, but that might be wrong. Can I ask for
a little more explanation from this perspective?

Thanks,
Naoya Horiguchi

> ---
>  mm/memory-failure.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 5a38e9eade94..78b173c7190c 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1885,6 +1885,12 @@ static int soft_offline_free_page(struct page *page)
>  	return rc;
>  }
>  
> +static void put_ref_page(struct page *page)
> +{
> +	if (page)
> +		put_page(page);
> +}
> +
>  /**
>   * soft_offline_page - Soft offline a page.
>   * @pfn: pfn to soft-offline
> @@ -1910,20 +1916,26 @@ static int soft_offline_free_page(struct page *page)
>  int soft_offline_page(unsigned long pfn, int flags)
>  {
>  	int ret;
> -	struct page *page;
>  	bool try_again = true;
> +	struct page *page, *ref_page = NULL;
> +
> +	WARN_ON_ONCE(!pfn_valid(pfn) && (flags & MF_COUNT_INCREASED));
>  
>  	if (!pfn_valid(pfn))
>  		return -ENXIO;
> +	if (flags & MF_COUNT_INCREASED)
> +		ref_page = pfn_to_page(pfn);
> +
>  	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
>  	page = pfn_to_online_page(pfn);
> -	if (!page)
> +	if (!page) {
> +		put_ref_page(ref_page);
>  		return -EIO;
> +	}
>  
>  	if (PageHWPoison(page)) {
>  		pr_info("%s: %#lx page already poisoned\n", __func__, pfn);
> -		if (flags & MF_COUNT_INCREASED)
> -			put_page(page);
> +		put_ref_page(ref_page);
>  		return 0;
>  	}
>  
> 

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[Dan Williams]

On Wed, Jan 13, 2021 at 5:50 PM HORIGUCHI NAOYA(堀口　直也)
<naoya.horiguchi@nec.com> wrote:
>
> On Wed, Jan 13, 2021 at 04:43:32PM -0800, Dan Williams wrote:
> > The conversion to move pfn_to_online_page() internal to
> > soft_offline_page() missed that the get_user_pages() reference taken by
> > the madvise() path needs to be dropped when pfn_to_online_page() fails.
> > Note the direct sysfs-path to soft_offline_page() does not perform a
> > get_user_pages() lookup.
> >
> > When soft_offline_page() is handed a pfn_valid() &&
> > !pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
> > a leaked reference.
> >
> > Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Oscar Salvador <osalvador@suse.de>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> I'm OK if we don't have any other better approach, but the proposed changes
> make code a little messy, and I feel that get_user_pages() might be the
> right place to fix. Is get_user_pages() expected to return struct page with
> holding refcount for offline valid pages?  I thought that such pages are
> only used by drivers for dax-devices, but that might be wrong. Can I ask for
> a little more explanation from this perspective?

The motivation for ZONE_DEVICE is to allow get_user_pages() for "offline" pfns.

soft_offline_page() wants to only operate on "online" pfns.

get_user_pages() on a dax-mapping returns an "offline" ZONE_DEVICE page.

When pfn_to_online_page() fails the get_user_pages() reference needs
to be dropped.

To be honest I dislike the entire flags based scheme for communicating
the fact that page reference obtained by madvise needs to be dropped
later. I'd rather pass a non-NULL 'struct page *' than redo
pfn_to_page() conversions in the leaf functions, but that's a much
larger change.

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[Oscar Salvador]

On 2021-01-14 07:18, Dan Williams wrote:
> To be honest I dislike the entire flags based scheme for communicating
> the fact that page reference obtained by madvise needs to be dropped
> later. I'd rather pass a non-NULL 'struct page *' than redo
> pfn_to_page() conversions in the leaf functions, but that's a much
> larger change.

We tried to remove that flag in the past but for different reasons.
I will have another look to see what can be done.

Thanks

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[HORIGUCHI NAOYA(堀口 直也)]

On Wed, Jan 13, 2021 at 10:18:09PM -0800, Dan Williams wrote:
> On Wed, Jan 13, 2021 at 5:50 PM HORIGUCHI NAOYA(堀口　直也)
> <naoya.horiguchi@nec.com> wrote:
> >
> > On Wed, Jan 13, 2021 at 04:43:32PM -0800, Dan Williams wrote:
> > > The conversion to move pfn_to_online_page() internal to
> > > soft_offline_page() missed that the get_user_pages() reference taken by
> > > the madvise() path needs to be dropped when pfn_to_online_page() fails.
> > > Note the direct sysfs-path to soft_offline_page() does not perform a
> > > get_user_pages() lookup.
> > >
> > > When soft_offline_page() is handed a pfn_valid() &&
> > > !pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
> > > a leaked reference.
> > >
> > > Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
> > > Cc: Michal Hocko <mhocko@kernel.org>
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Reviewed-by: Oscar Salvador <osalvador@suse.de>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >
> > I'm OK if we don't have any other better approach, but the proposed changes
> > make code a little messy, and I feel that get_user_pages() might be the
> > right place to fix. Is get_user_pages() expected to return struct page with
> > holding refcount for offline valid pages?  I thought that such pages are
> > only used by drivers for dax-devices, but that might be wrong. Can I ask for
> > a little more explanation from this perspective?
> 
> The motivation for ZONE_DEVICE is to allow get_user_pages() for "offline" pfns.

I missed this point, thank you.

> 
> soft_offline_page() wants to only operate on "online" pfns.

Right.

> 
> get_user_pages() on a dax-mapping returns an "offline" ZONE_DEVICE page.

OK.

> 
> When pfn_to_online_page() fails the get_user_pages() reference needs
> to be dropped.

The background is clear to me, and I agree with this patch.

Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

> 
> To be honest I dislike the entire flags based scheme for communicating
> the fact that page reference obtained by madvise needs to be dropped
> later. I'd rather pass a non-NULL 'struct page *' than redo
> pfn_to_page() conversions in the leaf functions, but that's a much
> larger change.

As Oscar mentions in another email, removing MF_COUNT_INCREASED was
recently discussed and rejected. I think that if pfn-page conversion
layer is somehow factored out from soft/hard offline handler, code would
be more readable/maintainable.

Thanks,
Naoya Horiguchi

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[Andrew Morton]

On Wed, 13 Jan 2021 16:43:32 -0800 Dan Williams <dan.j.williams@intel.com> wrote:

> The conversion to move pfn_to_online_page() internal to
> soft_offline_page() missed that the get_user_pages() reference taken by
> the madvise() path needs to be dropped when pfn_to_online_page() fails.
> Note the direct sysfs-path to soft_offline_page() does not perform a
> get_user_pages() lookup.
> 
> When soft_offline_page() is handed a pfn_valid() &&
> !pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
> a leaked reference.
> 
> Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A cc:stable patch in the middle is awkward.  I'll make this a
standalone patch for merging into mainline soon (for 5.11) and shall
turn the rest into a 4-patch series, OK?

Re: [PATCH v4 4/5] mm: Fix page reference leak in soft_offline_page()

[Dan Williams]

On Sun, Jan 17, 2021 at 2:01 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 13 Jan 2021 16:43:32 -0800 Dan Williams <dan.j.williams@intel.com> wrote:
>
> > The conversion to move pfn_to_online_page() internal to
> > soft_offline_page() missed that the get_user_pages() reference taken by
> > the madvise() path needs to be dropped when pfn_to_online_page() fails.
> > Note the direct sysfs-path to soft_offline_page() does not perform a
> > get_user_pages() lookup.
> >
> > When soft_offline_page() is handed a pfn_valid() &&
> > !pfn_to_online_page() pfn the kernel hangs at dax-device shutdown due to
> > a leaked reference.
> >
> > Fixes: feec24a6139d ("mm, soft-offline: convert parameter to pfn")
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Oscar Salvador <osalvador@suse.de>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> A cc:stable patch in the middle is awkward.  I'll make this a
> standalone patch for merging into mainline soon (for 5.11) and shall
> turn the rest into a 4-patch series, OK?

Sounds good to me.