On Fri, 1 Aug 2014, Naoya Horiguchi wrote:
> After fixing locking in follow_page(FOLL_GET) for hugepages, I start to
> observe the BUG of "get_page() on refcount 0 page" in hugetlb_fault() in
> the same test.
>
> I'm not exactly sure about how this race is triggered, but hugetlb_fault()
> calls pte_page() and get_page() outside page table lock, so it's not safe.
> This patch checks the refcount of the gotten page, and aborts the page fault
> if the refcount is 0, expecting to retry.
>
Fixes: 66aebce747ea ("hugetlb: fix race condition in hugetlb_fault()")
> Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # [3.12+]
I disagree with your 3.12+ annotation there: you may have hit the issue
in testing your hugepage migration work, but it's older than that: the
problematic get_page() was introduced in 3.4, and has been backported
to 3.2-stable: so 3.2+.
I was suspicious of this patch at first, then on the point of giving it
an Ack, and then realized that I had been right to be suspicious of it.
You're not the first the get the sequence wrong here; and it won't be
surprising if there are other instances of subtle get_page_unless_zero()
misuse elsewhere in the tree (I dare not look! someone else please do).
It's not the use of get_page_unless_zero() itself that is wrong, it's
the unjustified confidence in it: what's wrong is the lock_page() after.
As you have found, and acknowledged with get_page_unless_zero(), is
that the page here may be stale, it might be already freed, it might
be already reused. If reused, then its page_count will no longer be 0,
but the new user expects to have sole ownership of the page. The new
owner might be using __set_page_locked() (or one of the other nonatomic
flags operations), or "if (!trylock_page(newpage)) BUG()" like
migration's move_to_new_page().
We are dealing with a recently-hugetlb page here: that might make the
race I'm describing even less likely than with usual order:0 pages,
but I don't think it eliminates it.
What to do instead? The first answer that occurs to me is to move the
the pte_page,get_page down after the pte_same check inside the spin_lock,
and only then do trylock_page(), backing out to wait_on_page_locked and
retry or refault if not.
Though if doing that, it might be more sensible only to trylock_page
before dropping ptl inside hugetlb_cow(). That would be a bigger,
maybe harder to backport, rearrangement.
What do you think?
Hugh
> ---
> mm/hugetlb.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git mmotm-2014-07-22-15-58.orig/mm/hugetlb.c mmotm-2014-07-22-15-58/mm/hugetlb.c
> index 4437896cd6ed..863f45f63cd5 100644
> --- mmotm-2014-07-22-15-58.orig/mm/hugetlb.c
> +++ mmotm-2014-07-22-15-58/mm/hugetlb.c
> @@ -3189,7 +3189,8 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> * so no worry about deadlock.
> */
> page = pte_page(entry);
> - get_page(page);
> + if (!get_page_unless_zero(page))
> + goto out_put_pagecache;
> if (page != pagecache_page)
> lock_page(page);
>
> @@ -3215,15 +3216,14 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>
> out_ptl:
> spin_unlock(ptl);
> -
> + if (page != pagecache_page)
> + unlock_page(page);
> + put_page(page);
> +out_put_pagecache:
> if (pagecache_page) {
> unlock_page(pagecache_page);
> put_page(pagecache_page);
> }
> - if (page != pagecache_page)
> - unlock_page(page);
> - put_page(page);
> -
> out_mutex:
> mutex_unlock(&htlb_fault_mutex_table[hash]);
> return ret;
> --
> 1.9.3
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>