linux-kernel - Re: [PATCH] mm, hugetlb: fix resv_huge_pages underflow on UFFDIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61c78897-27a2-768b-f4fe-04e24b617ab6@oracle.com>
Date:   Thu, 20 May 2021 13:00:25 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Mina Almasry <almasrymina@...gle.com>
Cc:     Axel Rasmussen <axelrasmussen@...gle.com>,
        Peter Xu <peterx@...hat.com>, Linux-MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY

On 5/20/21 12:21 PM, Mina Almasry wrote:
> On Thu, May 20, 2021 at 12:18 PM Mina Almasry <almasrymina@...gle.com> wrote:
>>
>> On Thu, May 13, 2021 at 5:14 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>>>
>>> How about this approach?
>>> - Keep the check for hugetlbfs_pagecache_present in hugetlb_mcopy_atomic_pte
>>>   that you added.  That will catch the race where the page was added to
>>>   the cache before entering the routine.
>>> - With the above check in place, we only need to worry about the case
>>>   where copy_huge_page_from_user fails and we must drop locks.  In this
>>>   case we:
>>>   - Free the page previously allocated.
>>>   - Allocate a 'temporary' huge page without consuming reserves.  I'm
>>>     thinking of something similar to page migration.
>>>   - Drop the locks and let the copy_huge_page_from_user be done to the
>>>     temporary page.
>>>   - When reentering hugetlb_mcopy_atomic_pte after dropping locks (the
>>>     *pagep case) we need to once again check
>>>     hugetlbfs_pagecache_present.
>>>   - We then try to allocate the huge page which will consume the
>>>     reserve.  If successful, copy contents of temporary page to newly
>>>     allocated page.  Free temporary page.
>>>
>>> There may be issues with this, and I have not given it deep thought.  It
>>> does abuse the temporary huge page concept, but perhaps no more than
>>> page migration.  Things do slow down if the extra page allocation and
>>> copy is required, but that would only be the case if copy_huge_page_from_user
>>> needs to be done without locks.  Not sure, but hoping that is rare.
>>
>> Just following up this a bit: I've implemented this approach locally,
>> and with it it's passing the test as-is. When I hack the code such
>> that the copy in hugetlb_mcopy_atomic_pte() always fails, I run into
>> this edge case, which causes resv_huge_pages to underflow again (this
>> time permemantly):
>>
>> - hugetlb_no_page() is called on an index and a page is allocated and
>> inserted into the cache consuming the reservation.
>> - remove_huge_page() is called on this index and the page is removed from cache.
>> - hugetlb_mcopy_atomic_pte() is called on this index, we do not find
>> the page in the cache and we trigger this code patch and the copy
>> fails.
>> - The allocations in this code path seem to double consume the
>> reservation and resv_huge_pages underflows.
>>
>> I'm looking at this edge case to understand why a prior
>> remove_huge_page() causes my code to underflow resv_huge_pages.
>>
> 
> I should also mention, without a prior remove_huge_page() this code
> path works fine, so it seems the fact that the reservation is consumed
> before causes trouble, but I'm not sure why yet.
> 

Hi Mina,

How about quickly posting the code?  I may be able to provide more
suggestions if I can see the actual code.
-- 
Mike Kravetz