Re: [PATCH] huegtlbfs: fix page leak during migration of file pages

[Mike Kravetz]

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