Stable Archive on lore.kernel.org
 help / Atom feed
* [PATCH] huegtlbfs: fix page leak during migration of file pages
@ 2019-01-30 21:14 Mike Kravetz
       [not found] ` <20190131141238.6D6C220881@mail.kernel.org>
  2019-02-07 18:50 ` Mike Kravetz
  0 siblings, 2 replies; 11+ messages in thread
From: Mike Kravetz @ 2019-01-30 21:14 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: Michal Hocko, Naoya Horiguchi, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	Mike Kravetz, stable

Files can be created and mapped in an explicitly mounted hugetlbfs
filesystem.  If pages in such files are migrated, the filesystem
usage will not be decremented for the associated pages.  This can
result in mmap or page allocation failures as it appears there are
fewer pages in the filesystem than there should be.

For example, a test program which hole punches, faults and migrates
pages in such a file (1G in size) will eventually fail because it
can not allocate a page.  Reported counts and usage at time of failure:

node0
537	free_hugepages
1024	nr_hugepages
0	surplus_hugepages
node1
1000	free_hugepages
1024	nr_hugepages
0	surplus_hugepages

Filesystem                         Size  Used Avail Use% Mounted on
nodev                              4.0G  4.0G     0 100% /var/opt/hugepool

Note that the filesystem shows 4G of pages used, while actual usage is
511 pages (just under 1G).  Failed trying to allocate page 512.

If a hugetlb page is associated with an explicitly mounted filesystem,
this information in contained in the page_private field.  At migration
time, this information is not preserved.  To fix, simply transfer
page_private from old to new page at migration time if necessary. Also,
migrate_page_states() unconditionally clears page_private and PagePrivate
of the old page.  It is unlikely, but possible that these fields could
be non-NULL and are needed at hugetlb free page time.  So, do not touch
these fields for hugetlb pages.

Cc: <stable@vger.kernel.org>
Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 10 ++++++++++
 mm/migrate.c         | 10 ++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 32920a10100e..fb6de1db8806 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
 	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
 	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
+
+	/*
+	 * page_private is subpool pointer in hugetlb pages, transfer
+	 * if needed.
+	 */
+	if (page_private(page) && !page_private(newpage)) {
+		set_page_private(newpage, page_private(page));
+		set_page_private(page, 0);
+	}
+
 	if (mode != MIGRATE_SYNC_NO_COPY)
 		migrate_page_copy(newpage, page);
 	else
diff --git a/mm/migrate.c b/mm/migrate.c
index f7e4bfdc13b7..0d9708803553 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
 	 */
 	if (PageSwapCache(page))
 		ClearPageSwapCache(page);
-	ClearPagePrivate(page);
-	set_page_private(page, 0);
+	/*
+	 * Unlikely, but PagePrivate and page_private could potentially
+	 * contain information needed at hugetlb free page time.
+	 */
+	if (!PageHuge(page)) {
+		ClearPagePrivate(page);
+		set_page_private(page, 0);
+	}
 
 	/*
 	 * If any waiters have accumulated on the new page then
-- 
2.17.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
       [not found] ` <20190131141238.6D6C220881@mail.kernel.org>
@ 2019-02-01 22:36   ` Mike Kravetz
  0 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2019-02-01 22:36 UTC (permalink / raw)
  To: Sasha Levin, linux-mm, linux-kernel; +Cc: Michal Hocko, stable

On 1/31/19 6:12 AM, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 290408d4a250 hugetlb: hugepage migration core.
> 
> The bot has tested the following trees: v4.20.5, v4.19.18, v4.14.96, v4.9.153, v4.4.172, v3.18.133.
> 
> v4.20.5: Build OK!
> v4.19.18: Build OK!
> v4.14.96: Build OK!
> v4.9.153: Failed to apply! Possible dependencies:
>     2916ecc0f9d4 ("mm/migrate: new migrate mode MIGRATE_SYNC_NO_COPY")
> 
> v4.4.172: Failed to apply! Possible dependencies:
>     09cbfeaf1a5a ("mm, fs: get rid of PAGE_CACHE_* and page_cache_{get,release} macros")
>     0e749e54244e ("dax: increase granularity of dax_clear_blocks() operations")
>     2916ecc0f9d4 ("mm/migrate: new migrate mode MIGRATE_SYNC_NO_COPY")
>     2a28900be206 ("udf: Export superblock magic to userspace")
>     4420cfd3f51c ("staging: lustre: format properly all comment blocks for LNet core")
>     48b4800a1c6a ("zsmalloc: page migration support")
>     5057dcd0f1aa ("virtio_balloon: export 'available' memory to balloon statistics")
>     52db400fcd50 ("pmem, dax: clean up clear_pmem()")
>     5b7a487cf32d ("f2fs: add customized migrate_page callback")
>     5fd88337d209 ("staging: lustre: fix all conditional comparison to zero in LNet layer")
>     a188222b6ed2 ("net: Rename NETIF_F_ALL_CSUM to NETIF_F_CSUM_MASK")
>     b1123ea6d3b3 ("mm: balloon: use general non-lru movable page feature")
>     b2e0d1625e19 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
>     bda807d44454 ("mm: migrate: support non-lru movable page migration")
>     c8b8e32d700f ("direct-io: eliminate the offset argument to ->direct_IO")
>     d1a5f2b4d8a1 ("block: use DAX for partition table reads")
>     e10624f8c097 ("pmem: fail io-requests to known bad blocks")
> 
> v3.18.133: Failed to apply! Possible dependencies:
>     0722b1011a5f ("f2fs: set page private for inmemory pages for truncation")
>     1601839e9e5b ("f2fs: fix to release count of meta page in ->invalidatepage")
>     2916ecc0f9d4 ("mm/migrate: new migrate mode MIGRATE_SYNC_NO_COPY")
>     31a3268839c1 ("f2fs: cleanup if-statement of phase in gc_data_segment")
>     34ba94bac938 ("f2fs: do not make dirty any inmemory pages")
>     34d67debe02b ("f2fs: add infra struct and helper for inline dir")
>     4634d71ed190 ("f2fs: fix missing kmem_cache_free")
>     487261f39bcd ("f2fs: merge {invalidate,release}page for meta/node/data pages")
>     5b7a487cf32d ("f2fs: add customized migrate_page callback")
>     67298804f344 ("f2fs: introduce struct inode_management to wrap inner fields")
>     769ec6e5b7d4 ("f2fs: call radix_tree_preload before radix_tree_insert")
>     7dda2af83b2b ("f2fs: more fast lookup for gc_inode list")
>     8b26ef98da33 ("f2fs: use rw_semaphore for nat entry lock")
>     8c402946f074 ("f2fs: introduce the number of inode entries")
>     9be32d72becc ("f2fs: do retry operations with cond_resched")
>     9e4ded3f309e ("f2fs: activate f2fs_trace_pid")
>     d5053a34a9cc ("f2fs: introduce -o fastboot for reducing booting time only")
>     e5e7ea3c86e5 ("f2fs: control the memory footprint used by ino entries")
>     f68daeebba5a ("f2fs: keep PagePrivate during releasepage")
> 
> 
> How should we proceed with this patch?

Hello automated Sasha,

First, let's wait for review/ack.  However, the patch does not strictly
'depend' on the functionality of the commits in the lists above.  If/when
this goes upstream I can provide backports for 4.9, 4.4 and 3.18.

-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-01-30 21:14 [PATCH] huegtlbfs: fix page leak during migration of file pages Mike Kravetz
       [not found] ` <20190131141238.6D6C220881@mail.kernel.org>
@ 2019-02-07 18:50 ` Mike Kravetz
  2019-02-08  2:31   ` Naoya Horiguchi
  1 sibling, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2019-02-07 18:50 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: Michal Hocko, Naoya Horiguchi, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On 1/30/19 1:14 PM, Mike Kravetz wrote:
> Files can be created and mapped in an explicitly mounted hugetlbfs
> filesystem.  If pages in such files are migrated, the filesystem
> usage will not be decremented for the associated pages.  This can
> result in mmap or page allocation failures as it appears there are
> fewer pages in the filesystem than there should be.

Does anyone have a little time to take a look at this?

While migration of hugetlb pages 'should' not be a common issue, we
have seen it happen via soft memory errors/page poisoning in production
environments.  Didn't see a leak in that case as it was with pages in a
Sys V shared mem segment.  However, our DB code is starting to make use
of files in explicitly mounted hugetlbfs filesystems.  Therefore, we are
more likely to hit this bug in the field.
-- 
Mike Kravetz

> 
> For example, a test program which hole punches, faults and migrates
> pages in such a file (1G in size) will eventually fail because it
> can not allocate a page.  Reported counts and usage at time of failure:
> 
> node0
> 537	free_hugepages
> 1024	nr_hugepages
> 0	surplus_hugepages
> node1
> 1000	free_hugepages
> 1024	nr_hugepages
> 0	surplus_hugepages
> 
> Filesystem                         Size  Used Avail Use% Mounted on
> nodev                              4.0G  4.0G     0 100% /var/opt/hugepool
> 
> Note that the filesystem shows 4G of pages used, while actual usage is
> 511 pages (just under 1G).  Failed trying to allocate page 512.
> 
> If a hugetlb page is associated with an explicitly mounted filesystem,
> this information in contained in the page_private field.  At migration
> time, this information is not preserved.  To fix, simply transfer
> page_private from old to new page at migration time if necessary. Also,
> migrate_page_states() unconditionally clears page_private and PagePrivate
> of the old page.  It is unlikely, but possible that these fields could
> be non-NULL and are needed at hugetlb free page time.  So, do not touch
> these fields for hugetlb pages.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  fs/hugetlbfs/inode.c | 10 ++++++++++
>  mm/migrate.c         | 10 ++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 32920a10100e..fb6de1db8806 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
>  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
>  	if (rc != MIGRATEPAGE_SUCCESS)
>  		return rc;
> +
> +	/*
> +	 * page_private is subpool pointer in hugetlb pages, transfer
> +	 * if needed.
> +	 */
> +	if (page_private(page) && !page_private(newpage)) {
> +		set_page_private(newpage, page_private(page));
> +		set_page_private(page, 0);
> +	}
> +
>  	if (mode != MIGRATE_SYNC_NO_COPY)
>  		migrate_page_copy(newpage, page);
>  	else
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7e4bfdc13b7..0d9708803553 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
>  	 */
>  	if (PageSwapCache(page))
>  		ClearPageSwapCache(page);
> -	ClearPagePrivate(page);
> -	set_page_private(page, 0);
> +	/*
> +	 * Unlikely, but PagePrivate and page_private could potentially
> +	 * contain information needed at hugetlb free page time.
> +	 */
> +	if (!PageHuge(page)) {
> +		ClearPagePrivate(page);
> +		set_page_private(page, 0);
> +	}
>  
>  	/*
>  	 * If any waiters have accumulated on the new page then
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-07 18:50 ` Mike Kravetz
@ 2019-02-08  2:31   ` Naoya Horiguchi
  2019-02-08  5:50     ` Mike Kravetz
  0 siblings, 1 reply; 11+ messages in thread
From: Naoya Horiguchi @ 2019-02-08  2:31 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
> On 1/30/19 1:14 PM, Mike Kravetz wrote:
> > Files can be created and mapped in an explicitly mounted hugetlbfs
> > filesystem.  If pages in such files are migrated, the filesystem
> > usage will not be decremented for the associated pages.  This can
> > result in mmap or page allocation failures as it appears there are
> > fewer pages in the filesystem than there should be.
> 
> Does anyone have a little time to take a look at this?
> 
> While migration of hugetlb pages 'should' not be a common issue, we
> have seen it happen via soft memory errors/page poisoning in production
> environments.  Didn't see a leak in that case as it was with pages in a
> Sys V shared mem segment.  However, our DB code is starting to make use
> of files in explicitly mounted hugetlbfs filesystems.  Therefore, we are
> more likely to hit this bug in the field.

Hi Mike,

Thank you for finding/reporting the problem.
# sorry for my late response.

> 
> > 
> > For example, a test program which hole punches, faults and migrates
> > pages in such a file (1G in size) will eventually fail because it
> > can not allocate a page.  Reported counts and usage at time of failure:
> > 
> > node0
> > 537	free_hugepages
> > 1024	nr_hugepages
> > 0	surplus_hugepages
> > node1
> > 1000	free_hugepages
> > 1024	nr_hugepages
> > 0	surplus_hugepages
> > 
> > Filesystem                         Size  Used Avail Use% Mounted on
> > nodev                              4.0G  4.0G     0 100% /var/opt/hugepool
> > 
> > Note that the filesystem shows 4G of pages used, while actual usage is
> > 511 pages (just under 1G).  Failed trying to allocate page 512.
> > 
> > If a hugetlb page is associated with an explicitly mounted filesystem,
> > this information in contained in the page_private field.  At migration
> > time, this information is not preserved.  To fix, simply transfer
> > page_private from old to new page at migration time if necessary. Also,
> > migrate_page_states() unconditionally clears page_private and PagePrivate
> > of the old page.  It is unlikely, but possible that these fields could
> > be non-NULL and are needed at hugetlb free page time.  So, do not touch
> > these fields for hugetlb pages.
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> >  fs/hugetlbfs/inode.c | 10 ++++++++++
> >  mm/migrate.c         | 10 ++++++++--
> >  2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index 32920a10100e..fb6de1db8806 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
> >  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
> >  	if (rc != MIGRATEPAGE_SUCCESS)
> >  		return rc;
> > +
> > +	/*
> > +	 * page_private is subpool pointer in hugetlb pages, transfer
> > +	 * if needed.
> > +	 */
> > +	if (page_private(page) && !page_private(newpage)) {
> > +		set_page_private(newpage, page_private(page));
> > +		set_page_private(page, 0);

You don't have to copy PagePrivate flag?

> > +	}
> > +
> >  	if (mode != MIGRATE_SYNC_NO_COPY)
> >  		migrate_page_copy(newpage, page);
> >  	else
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7e4bfdc13b7..0d9708803553 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
> >  	 */
> >  	if (PageSwapCache(page))
> >  		ClearPageSwapCache(page);
> > -	ClearPagePrivate(page);
> > -	set_page_private(page, 0);
> > +	/*
> > +	 * Unlikely, but PagePrivate and page_private could potentially
> > +	 * contain information needed at hugetlb free page time.
> > +	 */
> > +	if (!PageHuge(page)) {
> > +		ClearPagePrivate(page);
> > +		set_page_private(page, 0);
> > +	}

# This argument is mainly for existing code...

According to the comment on migrate_page():

    /*
     * Common logic to directly migrate a single LRU page suitable for
     * pages that do not use PagePrivate/PagePrivate2.
     *
     * Pages are locked upon entry and exit.
     */
    int migrate_page(struct address_space *mapping, ...

So this common logic assumes that page_private is not used, so why do
we explicitly clear page_private in migrate_page_states()?
buffer_migrate_page(), which is commonly used for the case when
page_private is used, does that clearing outside migrate_page_states().
So I thought that hugetlbfs_migrate_page() could do in the similar manner.
IOW, migrate_page_states() should not do anything on PagePrivate.
But there're a few other .migratepage callbacks, and I'm not sure all of
them are safe for the change, so this approach might not fit for a small fix.

# BTW, there seems a typo in $SUBJECT.

Thanks,
Naoya Horiguchi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-08  2:31   ` Naoya Horiguchi
@ 2019-02-08  5:50     ` Mike Kravetz
  2019-02-08  7:31       ` Naoya Horiguchi
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2019-02-08  5:50 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
> On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
>> On 1/30/19 1:14 PM, Mike Kravetz wrote:
>>> +++ b/fs/hugetlbfs/inode.c
>>> @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
>>>  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
>>>  	if (rc != MIGRATEPAGE_SUCCESS)
>>>  		return rc;
>>> +
>>> +	/*
>>> +	 * page_private is subpool pointer in hugetlb pages, transfer
>>> +	 * if needed.
>>> +	 */
>>> +	if (page_private(page) && !page_private(newpage)) {
>>> +		set_page_private(newpage, page_private(page));
>>> +		set_page_private(page, 0);
> 
> You don't have to copy PagePrivate flag?
> 

Well my original thought was no.  For hugetlb pages, PagePrivate is not
associated with page_private.  It indicates a reservation was consumed.
It is set  when a hugetlb page is newly allocated and the allocation is
associated with a reservation and the global reservation count is
decremented.  When the page is added to the page cache or rmap,
PagePrivate is cleared.  If the page is free'ed before being added to page
cache or rmap, PagePrivate tells free_huge_page to restore (increment) the
reserve count as we did not 'instantiate' the page.

So, PagePrivate is only set from the time a huge page is allocated until
it is added to page cache or rmap.  My original thought was that the page
could not be migrated during this time.  However, I am not sure if that
reasoning is correct.  The page is not locked, so it would appear that it
could be migrated?  But, if it can be migrated at this time then perhaps
there are bigger issues for the (hugetlb) page fault code?

>>> +
>>> +	}
>>> +
>>>  	if (mode != MIGRATE_SYNC_NO_COPY)
>>>  		migrate_page_copy(newpage, page);
>>>  	else
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index f7e4bfdc13b7..0d9708803553 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
>>>  	 */
>>>  	if (PageSwapCache(page))
>>>  		ClearPageSwapCache(page);
>>> -	ClearPagePrivate(page);
>>> -	set_page_private(page, 0);
>>> +	/*
>>> +	 * Unlikely, but PagePrivate and page_private could potentially
>>> +	 * contain information needed at hugetlb free page time.
>>> +	 */
>>> +	if (!PageHuge(page)) {
>>> +		ClearPagePrivate(page);
>>> +		set_page_private(page, 0);
>>> +	}
> 
> # This argument is mainly for existing code...
> 
> According to the comment on migrate_page():
> 
>     /*
>      * Common logic to directly migrate a single LRU page suitable for
>      * pages that do not use PagePrivate/PagePrivate2.
>      *
>      * Pages are locked upon entry and exit.
>      */
>     int migrate_page(struct address_space *mapping, ...
> 
> So this common logic assumes that page_private is not used, so why do
> we explicitly clear page_private in migrate_page_states()?

Perhaps someone else knows.  If not, I can do some git research and
try to find out why.

> buffer_migrate_page(), which is commonly used for the case when
> page_private is used, does that clearing outside migrate_page_states().
> So I thought that hugetlbfs_migrate_page() could do in the similar manner.
> IOW, migrate_page_states() should not do anything on PagePrivate.
> But there're a few other .migratepage callbacks, and I'm not sure all of
> them are safe for the change, so this approach might not fit for a small fix.

I will look at those as well unless someone knows without researching.

> 
> # BTW, there seems a typo in $SUBJECT.

Thanks!

-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-08  5:50     ` Mike Kravetz
@ 2019-02-08  7:31       ` Naoya Horiguchi
  2019-02-11 23:06         ` Mike Kravetz
  0 siblings, 1 reply; 11+ messages in thread
From: Naoya Horiguchi @ 2019-02-08  7:31 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
> On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
> > On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
> >> On 1/30/19 1:14 PM, Mike Kravetz wrote:
> >>> +++ b/fs/hugetlbfs/inode.c
> >>> @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
> >>>  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
> >>>  	if (rc != MIGRATEPAGE_SUCCESS)
> >>>  		return rc;
> >>> +
> >>> +	/*
> >>> +	 * page_private is subpool pointer in hugetlb pages, transfer
> >>> +	 * if needed.
> >>> +	 */
> >>> +	if (page_private(page) && !page_private(newpage)) {
> >>> +		set_page_private(newpage, page_private(page));
> >>> +		set_page_private(page, 0);
> > 
> > You don't have to copy PagePrivate flag?
> > 
> 
> Well my original thought was no.  For hugetlb pages, PagePrivate is not
> associated with page_private.  It indicates a reservation was consumed.
> It is set  when a hugetlb page is newly allocated and the allocation is
> associated with a reservation and the global reservation count is
> decremented.  When the page is added to the page cache or rmap,
> PagePrivate is cleared.  If the page is free'ed before being added to page
> cache or rmap, PagePrivate tells free_huge_page to restore (increment) the
> reserve count as we did not 'instantiate' the page.
> 
> So, PagePrivate is only set from the time a huge page is allocated until
> it is added to page cache or rmap.  My original thought was that the page
> could not be migrated during this time.  However, I am not sure if that
> reasoning is correct.  The page is not locked, so it would appear that it
> could be migrated?  But, if it can be migrated at this time then perhaps
> there are bigger issues for the (hugetlb) page fault code?

In my understanding, free hugetlb pages are not expected to be passed to
migrate_pages(), and currently that's ensured by each migration caller
which checks and avoids free hugetlb pages on its own.
migrate_pages() and its internal code are probably not aware of handling
free hugetlb pages, so if they are accidentally passed to migration code,
that's a big problem as you are concerned.
So the above reasoning should work at least this assumption is correct.

Most of migration callers are not intersted in moving free hugepages.
The one I'm not sure of is the code path from alloc_contig_range().
If someone think it's worthwhile to migrate free hugepage to get bigger
contiguous memory, he/she tries to enable that code path and the assumption
will be broken.

Thanks,
Naoya Horiguchi

> 
> >>> +
> >>> +	}
> >>> +
> >>>  	if (mode != MIGRATE_SYNC_NO_COPY)
> >>>  		migrate_page_copy(newpage, page);
> >>>  	else
> >>> diff --git a/mm/migrate.c b/mm/migrate.c
> >>> index f7e4bfdc13b7..0d9708803553 100644
> >>> --- a/mm/migrate.c
> >>> +++ b/mm/migrate.c
> >>> @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
> >>>  	 */
> >>>  	if (PageSwapCache(page))
> >>>  		ClearPageSwapCache(page);
> >>> -	ClearPagePrivate(page);
> >>> -	set_page_private(page, 0);
> >>> +	/*
> >>> +	 * Unlikely, but PagePrivate and page_private could potentially
> >>> +	 * contain information needed at hugetlb free page time.
> >>> +	 */
> >>> +	if (!PageHuge(page)) {
> >>> +		ClearPagePrivate(page);
> >>> +		set_page_private(page, 0);
> >>> +	}
> > 
> > # This argument is mainly for existing code...
> > 
> > According to the comment on migrate_page():
> > 
> >     /*
> >      * Common logic to directly migrate a single LRU page suitable for
> >      * pages that do not use PagePrivate/PagePrivate2.
> >      *
> >      * Pages are locked upon entry and exit.
> >      */
> >     int migrate_page(struct address_space *mapping, ...
> > 
> > So this common logic assumes that page_private is not used, so why do
> > we explicitly clear page_private in migrate_page_states()?
> 
> Perhaps someone else knows.  If not, I can do some git research and
> try to find out why.
> 
> > buffer_migrate_page(), which is commonly used for the case when
> > page_private is used, does that clearing outside migrate_page_states().
> > So I thought that hugetlbfs_migrate_page() could do in the similar manner.
> > IOW, migrate_page_states() should not do anything on PagePrivate.
> > But there're a few other .migratepage callbacks, and I'm not sure all of
> > them are safe for the change, so this approach might not fit for a small fix.
> 
> I will look at those as well unless someone knows without researching.
> 
> > 
> > # BTW, there seems a typo in $SUBJECT.
> 
> Thanks!
> 
> -- 
> Mike Kravetz
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-08  7:31       ` Naoya Horiguchi
@ 2019-02-11 23:06         ` Mike Kravetz
  2019-02-12  2:24           ` Naoya Horiguchi
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2019-02-11 23:06 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On 2/7/19 11:31 PM, Naoya Horiguchi wrote:
> On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
>> On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
>>> On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
>>>> On 1/30/19 1:14 PM, Mike Kravetz wrote:
>>>>> +++ b/fs/hugetlbfs/inode.c
>>>>> @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
>>>>>  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
>>>>>  	if (rc != MIGRATEPAGE_SUCCESS)
>>>>>  		return rc;
>>>>> +
>>>>> +	/*
>>>>> +	 * page_private is subpool pointer in hugetlb pages, transfer
>>>>> +	 * if needed.
>>>>> +	 */
>>>>> +	if (page_private(page) && !page_private(newpage)) {
>>>>> +		set_page_private(newpage, page_private(page));
>>>>> +		set_page_private(page, 0);
>>>
>>> You don't have to copy PagePrivate flag?
>>>
>>
>> Well my original thought was no.  For hugetlb pages, PagePrivate is not
>> associated with page_private.  It indicates a reservation was consumed.
>> It is set  when a hugetlb page is newly allocated and the allocation is
>> associated with a reservation and the global reservation count is
>> decremented.  When the page is added to the page cache or rmap,
>> PagePrivate is cleared.  If the page is free'ed before being added to page
>> cache or rmap, PagePrivate tells free_huge_page to restore (increment) the
>> reserve count as we did not 'instantiate' the page.
>>
>> So, PagePrivate is only set from the time a huge page is allocated until
>> it is added to page cache or rmap.  My original thought was that the page
>> could not be migrated during this time.  However, I am not sure if that
>> reasoning is correct.  The page is not locked, so it would appear that it
>> could be migrated?  But, if it can be migrated at this time then perhaps
>> there are bigger issues for the (hugetlb) page fault code?
> 
> In my understanding, free hugetlb pages are not expected to be passed to
> migrate_pages(), and currently that's ensured by each migration caller
> which checks and avoids free hugetlb pages on its own.
> migrate_pages() and its internal code are probably not aware of handling
> free hugetlb pages, so if they are accidentally passed to migration code,
> that's a big problem as you are concerned.
> So the above reasoning should work at least this assumption is correct.
> 
> Most of migration callers are not intersted in moving free hugepages.
> The one I'm not sure of is the code path from alloc_contig_range().
> If someone think it's worthwhile to migrate free hugepage to get bigger
> contiguous memory, he/she tries to enable that code path and the assumption
> will be broken.

You are correct.  We do not migrate free huge pages.  I was thinking more
about problems if we migrate a page while it is being added to a task's page
table as in hugetlb_no_page.

Commit bcc54222309c ("mm: hugetlb: introduce page_huge_active") addresses
this issue, but I believe there is a bug in the implementation.
isolate_huge_page contains this test:

	if (!page_huge_active(page) || !get_page_unless_zero(page)) {
		ret = false;
		goto unlock;
	}

If the condition is not met, then the huge page can be isolated and migrated.

In hugetlb_no_page, there is this block of code:

                page = alloc_huge_page(vma, haddr, 0);
                if (IS_ERR(page)) {
                        ret = vmf_error(PTR_ERR(page));
                        goto out;
                }
                clear_huge_page(page, address, pages_per_huge_page(h));
                __SetPageUptodate(page);
                set_page_huge_active(page);

                if (vma->vm_flags & VM_MAYSHARE) {
                        int err = huge_add_to_page_cache(page, mapping, idx);
                        if (err) {
                                put_page(page);
                                if (err == -EEXIST)
                                        goto retry;
                                goto out;
                        }
                } else {
                        lock_page(page);
                        if (unlikely(anon_vma_prepare(vma))) {
                                ret = VM_FAULT_OOM;
                                goto backout_unlocked;
                        }
                        anon_rmap = 1;
                }
        } else {

Note that we call set_page_huge_active BEFORE locking the page.  This
means that we can isolate the page and have migration take place while
we continue to add the page to page tables.  I was able to make this
happen by adding a udelay() after set_page_huge_active to simulate worst
case scheduling behavior.  It resulted in VM_BUG_ON while unlocking page.
My test had several threads faulting in huge pages.  Another thread was
offlining the memory blocks forcing migration.

To fix this, we need to delay the set_page_huge_active call until after
the page is locked.  I am testing a patch with this change.  Perhaps we
should even delay calling set_page_huge_active until we know there are
no errors and we know the page is actually in page tables?

While looking at this, I think there is another issue.  When a hugetlb
page is migrated, we do not migrate the 'page_huge_active' state of the
page.  That should be moved as the page is migrated.  Correct?
-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-11 23:06         ` Mike Kravetz
@ 2019-02-12  2:24           ` Naoya Horiguchi
  2019-02-12  2:37             ` Mike Kravetz
  0 siblings, 1 reply; 11+ messages in thread
From: Naoya Horiguchi @ 2019-02-12  2:24 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On Mon, Feb 11, 2019 at 03:06:27PM -0800, Mike Kravetz wrote:
> On 2/7/19 11:31 PM, Naoya Horiguchi wrote:
> > On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
> >> On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
> >>> On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
> >>>> On 1/30/19 1:14 PM, Mike Kravetz wrote:
> >>>>> +++ b/fs/hugetlbfs/inode.c
> >>>>> @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
> >>>>>  	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
> >>>>>  	if (rc != MIGRATEPAGE_SUCCESS)
> >>>>>  		return rc;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * page_private is subpool pointer in hugetlb pages, transfer
> >>>>> +	 * if needed.
> >>>>> +	 */
> >>>>> +	if (page_private(page) && !page_private(newpage)) {
> >>>>> +		set_page_private(newpage, page_private(page));
> >>>>> +		set_page_private(page, 0);
> >>>
> >>> You don't have to copy PagePrivate flag?
> >>>
> >>
> >> Well my original thought was no.  For hugetlb pages, PagePrivate is not
> >> associated with page_private.  It indicates a reservation was consumed.
> >> It is set  when a hugetlb page is newly allocated and the allocation is
> >> associated with a reservation and the global reservation count is
> >> decremented.  When the page is added to the page cache or rmap,
> >> PagePrivate is cleared.  If the page is free'ed before being added to page
> >> cache or rmap, PagePrivate tells free_huge_page to restore (increment) the
> >> reserve count as we did not 'instantiate' the page.
> >>
> >> So, PagePrivate is only set from the time a huge page is allocated until
> >> it is added to page cache or rmap.  My original thought was that the page
> >> could not be migrated during this time.  However, I am not sure if that
> >> reasoning is correct.  The page is not locked, so it would appear that it
> >> could be migrated?  But, if it can be migrated at this time then perhaps
> >> there are bigger issues for the (hugetlb) page fault code?
> > 
> > In my understanding, free hugetlb pages are not expected to be passed to
> > migrate_pages(), and currently that's ensured by each migration caller
> > which checks and avoids free hugetlb pages on its own.
> > migrate_pages() and its internal code are probably not aware of handling
> > free hugetlb pages, so if they are accidentally passed to migration code,
> > that's a big problem as you are concerned.
> > So the above reasoning should work at least this assumption is correct.
> > 
> > Most of migration callers are not intersted in moving free hugepages.
> > The one I'm not sure of is the code path from alloc_contig_range().
> > If someone think it's worthwhile to migrate free hugepage to get bigger
> > contiguous memory, he/she tries to enable that code path and the assumption
> > will be broken.
> 
> You are correct.  We do not migrate free huge pages.  I was thinking more
> about problems if we migrate a page while it is being added to a task's page
> table as in hugetlb_no_page.
> 
> Commit bcc54222309c ("mm: hugetlb: introduce page_huge_active") addresses
> this issue, but I believe there is a bug in the implementation.
> isolate_huge_page contains this test:
> 
> 	if (!page_huge_active(page) || !get_page_unless_zero(page)) {
> 		ret = false;
> 		goto unlock;
> 	}
> 
> If the condition is not met, then the huge page can be isolated and migrated.
> 
> In hugetlb_no_page, there is this block of code:
> 
>                 page = alloc_huge_page(vma, haddr, 0);
>                 if (IS_ERR(page)) {
>                         ret = vmf_error(PTR_ERR(page));
>                         goto out;
>                 }
>                 clear_huge_page(page, address, pages_per_huge_page(h));
>                 __SetPageUptodate(page);
>                 set_page_huge_active(page);
> 
>                 if (vma->vm_flags & VM_MAYSHARE) {
>                         int err = huge_add_to_page_cache(page, mapping, idx);
>                         if (err) {
>                                 put_page(page);
>                                 if (err == -EEXIST)
>                                         goto retry;
>                                 goto out;
>                         }
>                 } else {
>                         lock_page(page);
>                         if (unlikely(anon_vma_prepare(vma))) {
>                                 ret = VM_FAULT_OOM;
>                                 goto backout_unlocked;
>                         }
>                         anon_rmap = 1;
>                 }
>         } else {
> 
> Note that we call set_page_huge_active BEFORE locking the page.  This
> means that we can isolate the page and have migration take place while
> we continue to add the page to page tables.  I was able to make this
> happen by adding a udelay() after set_page_huge_active to simulate worst
> case scheduling behavior.  It resulted in VM_BUG_ON while unlocking page.
> My test had several threads faulting in huge pages.  Another thread was
> offlining the memory blocks forcing migration.

This shows another problem, so I agree we need a fix.

> 
> To fix this, we need to delay the set_page_huge_active call until after
> the page is locked.  I am testing a patch with this change.  Perhaps we
> should even delay calling set_page_huge_active until we know there are
> no errors and we know the page is actually in page tables?

Yes, calling set_page_huge_active after page table is set up sounds nice to me.

> 
> While looking at this, I think there is another issue.  When a hugetlb
> page is migrated, we do not migrate the 'page_huge_active' state of the
> page.  That should be moved as the page is migrated.  Correct?

Yes, and I think that putback_active_hugepage(new_hpage) at the last step
of migration sequence handles the copying of 'page_huge_active' state.

Thanks,
Naoya Horiguchi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix page leak during migration of file pages
  2019-02-12  2:24           ` Naoya Horiguchi
@ 2019-02-12  2:37             ` Mike Kravetz
  2019-02-12 22:14               ` [PATCH] huegtlbfs: fix races and page leaks during migration Mike Kravetz
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2019-02-12  2:37 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: linux-mm, linux-kernel, Michal Hocko, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	stable

On 2/11/19 6:24 PM, Naoya Horiguchi wrote:
> On Mon, Feb 11, 2019 at 03:06:27PM -0800, Mike Kravetz wrote:
>> While looking at this, I think there is another issue.  When a hugetlb
>> page is migrated, we do not migrate the 'page_huge_active' state of the
>> page.  That should be moved as the page is migrated.  Correct?
> 
> Yes, and I think that putback_active_hugepage(new_hpage) at the last step
> of migration sequence handles the copying of 'page_huge_active' state.
> 

Thanks!  I missed the putback_active_hugepage that takes care of making
the target migration page active.

-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] huegtlbfs: fix races and page leaks during migration
  2019-02-12  2:37             ` Mike Kravetz
@ 2019-02-12 22:14               ` Mike Kravetz
  2019-02-14  1:32                 ` Mike Kravetz
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2019-02-12 22:14 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: Michal Hocko, Naoya Horiguchi, Andrea Arcangeli,
	Kirill A . Shutemov, Mel Gorman, Davidlohr Bueso, Andrew Morton,
	Mike Kravetz, stable

hugetlb pages should only be migrated if they are 'active'.  The routines
set/clear_page_huge_active() modify the active state of hugetlb pages.
When a new hugetlb page is allocated at fault time, set_page_huge_active
is called before the page is locked.  Therefore, another thread could
race and migrate the page while it is being added to page table by the
fault code.  This race is somewhat hard to trigger, but can be seen by
strategically adding udelay to simulate worst case scheduling behavior.
Depending on 'how' the code races, various BUG()s could be triggered.

To address this issue, simply delay the set_page_huge_active call until
after the page is successfully added to the page table.

Hugetlb pages can also be leaked at migration time if the pages are
associated with a file in an explicitly mounted hugetlbfs filesystem.
For example, a test program which hole punches, faults and migrates
pages in such a file (1G in size) will eventually fail because it
can not allocate a page.  Reported counts and usage at time of failure:

node0
537     free_hugepages
1024    nr_hugepages
0       surplus_hugepages
node1
1000    free_hugepages
1024    nr_hugepages
0       surplus_hugepages

Filesystem                         Size  Used Avail Use% Mounted on
nodev                              4.0G  4.0G     0 100% /var/opt/hugepool

Note that the filesystem shows 4G of pages used, while actual usage is
511 pages (just under 1G).  Failed trying to allocate page 512.

If a hugetlb page is associated with an explicitly mounted filesystem,
this information in contained in the page_private field.  At migration
time, this information is not preserved.  To fix, simply transfer
page_private from old to new page at migration time if necessary.

Cc: <stable@vger.kernel.org>
Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 12 ++++++++++++
 mm/hugetlb.c         |  9 ++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 32920a10100e..a7fa037b876b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
 	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
 	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
+
+	/*
+	 * page_private is subpool pointer in hugetlb pages.  Transfer to
+	 * new page.  PagePrivate is not associated with page_private for
+	 * hugetlb pages and can not be set here as only page_huge_active
+	 * pages can be migrated.
+	 */
+	if (page_private(page)) {
+		set_page_private(newpage, page_private(page));
+		set_page_private(page, 0);
+	}
+
 	if (mode != MIGRATE_SYNC_NO_COPY)
 		migrate_page_copy(newpage, page);
 	else
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a80832487981..f859e319e3eb 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 	copy_user_huge_page(new_page, old_page, address, vma,
 			    pages_per_huge_page(h));
 	__SetPageUptodate(new_page);
-	set_page_huge_active(new_page);
 
 	mmun_start = haddr;
 	mmun_end = mmun_start + huge_page_size(h);
@@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page, true);
 		hugepage_add_new_anon_rmap(new_page, vma, haddr);
+		set_page_huge_active(new_page);
 		/* Make the old page be freed below */
 		new_page = old_page;
 	}
@@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 		}
 		clear_huge_page(page, address, pages_per_huge_page(h));
 		__SetPageUptodate(page);
-		set_page_huge_active(page);
 
 		if (vma->vm_flags & VM_MAYSHARE) {
 			int err = huge_add_to_page_cache(page, mapping, idx);
@@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 	}
 
 	spin_unlock(ptl);
+
+	/* May already be set if not newly allocated page */
+	set_page_huge_active(page);
+
 	unlock_page(page);
 out:
 	return ret;
@@ -4097,7 +4100,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	 * the set_pte_at() write.
 	 */
 	__SetPageUptodate(page);
-	set_page_huge_active(page);
 
 	mapping = dst_vma->vm_file->f_mapping;
 	idx = vma_hugecache_offset(h, dst_vma, dst_addr);
@@ -4165,6 +4167,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	update_mmu_cache(dst_vma, dst_addr, dst_pte);
 
 	spin_unlock(ptl);
+	set_page_huge_active(page);
 	if (vm_shared)
 		unlock_page(page);
 	ret = 0;
-- 
2.17.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] huegtlbfs: fix races and page leaks during migration
  2019-02-12 22:14               ` [PATCH] huegtlbfs: fix races and page leaks during migration Mike Kravetz
@ 2019-02-14  1:32                 ` Mike Kravetz
  0 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2019-02-14  1:32 UTC (permalink / raw)
  To: linux-mm, linux-kernel, Naoya Horiguchi, Andrew Morton
  Cc: Michal Hocko, Andrea Arcangeli, Kirill A . Shutemov, Mel Gorman,
	Davidlohr Bueso, stable

On 2/12/19 2:14 PM, Mike Kravetz wrote:
> 
> Hugetlb pages can also be leaked at migration time if the pages are
> associated with a file in an explicitly mounted hugetlbfs filesystem.
> For example, a test program which hole punches, faults and migrates
> pages in such a file (1G in size) will eventually fail because it
> can not allocate a page.  Reported counts and usage at time of failure:
> 
> node0
> 537     free_hugepages
> 1024    nr_hugepages
> 0       surplus_hugepages
> node1
> 1000    free_hugepages
> 1024    nr_hugepages
> 0       surplus_hugepages
> 
> Filesystem                         Size  Used Avail Use% Mounted on
> nodev                              4.0G  4.0G     0 100% /var/opt/hugepool
> 
> Note that the filesystem shows 4G of pages used, while actual usage is
> 511 pages (just under 1G).  Failed trying to allocate page 512.

My apologies.  The test scenario described above does not trigger the
page leak issue fixed with this patch.  It actually triggers another
undiagnosed and unfixed issue with huge page migration that I will
be working on.  Sigh!

The leak with migration of huge pages in explicitly mounted filesystem
is still fixed by this patch.  However, the commit message should be
changed to more accurately reflect testing and observed outcomes.  The
patch with only commit message changes is below:

From: Mike Kravetz <mike.kravetz@oracle.com>
Date: Tue, 12 Feb 2019 10:58:28 -0800
Subject: [PATCH] huegtlbfs: fix races and page leaks during migration

hugetlb pages should only be migrated if they are 'active'.  The routines
set/clear_page_huge_active() modify the active state of hugetlb pages.
When a new hugetlb page is allocated at fault time, set_page_huge_active
is called before the page is locked.  Therefore, another thread could
race and migrate the page while it is being added to page table by the
fault code.  This race is somewhat hard to trigger, but can be seen by
strategically adding udelay to simulate worst case scheduling behavior.
Depending on 'how' the code races, various BUG()s could be triggered.

To address this issue, simply delay the set_page_huge_active call until
after the page is successfully added to the page table.

Hugetlb pages can also be leaked at migration time if the pages are
associated with a file in an explicitly mounted hugetlbfs filesystem.
For example, consider a two node system with 4GB worth of huge pages
available.  A program mmaps a 2G file in a hugetlbfs filesystem.  It
then migrates the pages associated with the file from one node to
another.  When the program exits, huge page counts are as follows:

node0
1024    free_hugepages
1024    nr_hugepages

node1
0       free_hugepages
1024    nr_hugepages

Filesystem                         Size  Used Avail Use% Mounted on
nodev                              4.0G  2.0G  2.0G  50% /var/opt/hugepool

That is as expected.  2G of huge pages are taken from the free_hugepages
counts, and 2G is the size of the file in the explicitly mounted filesystem.
If the file is then removed, the counts become:

node0
1024    free_hugepages
1024    nr_hugepages

node1
1024    free_hugepages
1024    nr_hugepages

Filesystem                         Size  Used Avail Use% Mounted on
nodev                              4.0G  2.0G  2.0G  50% /var/opt/hugepool

Note that the filesystem still shows 2G of pages used, while there
actually are no huge pages in use.  The only way to 'fix' the
filesystem accounting is to unmount the filesystem

If a hugetlb page is associated with an explicitly mounted filesystem,
this information in contained in the page_private field.  At migration
time, this information is not preserved.  To fix, simply transfer
page_private from old to new page at migration time if necessary.

Cc: <stable@vger.kernel.org>
Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 12 ++++++++++++
 mm/hugetlb.c         |  9 ++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 32920a10100e..a7fa037b876b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space
*mapping,
 	rc = migrate_huge_page_move_mapping(mapping, newpage, page);
 	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
+
+	/*
+	 * page_private is subpool pointer in hugetlb pages.  Transfer to
+	 * new page.  PagePrivate is not associated with page_private for
+	 * hugetlb pages and can not be set here as only page_huge_active
+	 * pages can be migrated.
+	 */
+	if (page_private(page)) {
+		set_page_private(newpage, page_private(page));
+		set_page_private(page, 0);
+	}
+
 	if (mode != MIGRATE_SYNC_NO_COPY)
 		migrate_page_copy(newpage, page);
 	else
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a80832487981..f859e319e3eb 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct
vm_area_struct *vma,
 	copy_user_huge_page(new_page, old_page, address, vma,
 			    pages_per_huge_page(h));
 	__SetPageUptodate(new_page);
-	set_page_huge_active(new_page);

 	mmun_start = haddr;
 	mmun_end = mmun_start + huge_page_size(h);
@@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct
vm_area_struct *vma,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page, true);
 		hugepage_add_new_anon_rmap(new_page, vma, haddr);
+		set_page_huge_active(new_page);
 		/* Make the old page be freed below */
 		new_page = old_page;
 	}
@@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 		}
 		clear_huge_page(page, address, pages_per_huge_page(h));
 		__SetPageUptodate(page);
-		set_page_huge_active(page);

 		if (vma->vm_flags & VM_MAYSHARE) {
 			int err = huge_add_to_page_cache(page, mapping, idx);
@@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 	}

 	spin_unlock(ptl);
+
+	/* May already be set if not newly allocated page */
+	set_page_huge_active(page);
+
 	unlock_page(page);
 out:
 	return ret;
@@ -4097,7 +4100,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	 * the set_pte_at() write.
 	 */
 	__SetPageUptodate(page);
-	set_page_huge_active(page);

 	mapping = dst_vma->vm_file->f_mapping;
 	idx = vma_hugecache_offset(h, dst_vma, dst_addr);
@@ -4165,6 +4167,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	update_mmu_cache(dst_vma, dst_addr, dst_pte);

 	spin_unlock(ptl);
+	set_page_huge_active(page);
 	if (vm_shared)
 		unlock_page(page);
 	ret = 0;
-- 
2.17.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-30 21:14 [PATCH] huegtlbfs: fix page leak during migration of file pages Mike Kravetz
     [not found] ` <20190131141238.6D6C220881@mail.kernel.org>
2019-02-01 22:36   ` Mike Kravetz
2019-02-07 18:50 ` Mike Kravetz
2019-02-08  2:31   ` Naoya Horiguchi
2019-02-08  5:50     ` Mike Kravetz
2019-02-08  7:31       ` Naoya Horiguchi
2019-02-11 23:06         ` Mike Kravetz
2019-02-12  2:24           ` Naoya Horiguchi
2019-02-12  2:37             ` Mike Kravetz
2019-02-12 22:14               ` [PATCH] huegtlbfs: fix races and page leaks during migration Mike Kravetz
2019-02-14  1:32                 ` Mike Kravetz

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox