From: Zi Yan <ziy@nvidia.com>
To: John Hubbard <jhubbard@nvidia.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>,
Andrew Morton <akpm@linux-foundation.org>,
Zi Yan <zi.yan@cs.rutgers.edu>,
"Aneesh Kumar K.V" <aneesh.kumar@kernel.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and GUP-fast
Date: Sat, 27 Apr 2024 16:45:33 -0400 [thread overview]
Message-ID: <E18F9D0B-C855-4252-BEEB-F8B1FA5BCE9E@nvidia.com> (raw)
In-Reply-To: <328b2b69-e4ab-4a00-9137-1f7c3dc2d195@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 6924 bytes --]
On 27 Apr 2024, at 15:11, John Hubbard wrote:
> On 4/27/24 8:14 AM, Zi Yan wrote:
>> On 27 Apr 2024, at 0:41, John Hubbard wrote:
>>> On 4/25/24 10:07 AM, Ryan Roberts wrote:
>>>> __split_huge_pmd_locked() can be called for a present THP, devmap or
>>>> (non-present) migration entry. It calls pmdp_invalidate()
>>>> unconditionally on the pmdp and only determines if it is present or not
>>>> based on the returned old pmd. This is a problem for the migration entry
>>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must only be
>>>> called for a present pmd.
>>>>
>>>> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any
>>>> future call to pmd_present() will return true. And therefore any
>>>> lockless pgtable walker could see the migration entry pmd in this state
>>>> and start interpretting the fields as if it were present, leading to
>>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker.
>>>> I suspect the same is possible on other architectures.
>>>>
>>>> Fix this by only calling pmdp_invalidate() for a present pmd. And for
>>>
>>> Yes, this seems like a good design decision (after reading through the
>>> discussion that you all had in the other threads).
>>
>> This will only be good for arm64 and does not prevent other arch developers
>> to write code breaking arm64, since only arm64's pmd_mkinvalid() can turn
>> a swap entry to a pmd_present() entry.
>
> Well, let's characterize it in a bit more detail, then:
>
> 1) This patch will make things better for arm64. That's important!
>
> 2) Equally important, this patch does not make anything worse for
> other CPU arches.
>
> 3) This patch represents a new design constraint on the CPU arch
> layer, and thus requires documentation and whatever enforcement
> we can provide, in order to keep future code out of trouble.
>
> 3.a) See the VM_WARN_ON() hunks below.
>
> 3.b) I like the new design constraint, because it is reasonable and
> clearly understandable: don't invalidate a non-present page
> table entry.
>
> I do wonder if there is somewhere else that this should be documented?
The issue is pmd_mkinvalid(), since it turns a swap entry into a pmd_present()
entry on arm64. This patch only adds a warning on pmd_invalidate(), although
pmd_invalidate() is the only caller of pmd_mkinvalid(). This means any
future user of pmd_mkinvalid() can cause the same issue on arm64 without any
warning.
I am not against changing the logic in __split_huge_pmd_lock() to fix arm64,
but just want to prevent future errors, that might only be possible on arm64.
BTW, in terms of the patch itself, moving "pmdp_invalidate(vma, haddr, pmd)"
without moving the big comment above it is not OK, since later no one can
figure out why that comment is there.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
>
>>>
>>>> good measure let's add a warning to the generic implementation of
>>>> pmdp_invalidate(). I've manually reviewed all other
>>>> pmdp_invalidate[_ad]() call sites and believe all others to be
>>>> conformant.
>>>>
>>>> This is a theoretical bug found during code review. I don't have any
>>>> test case to trigger it in practice.
>>>>
>>>> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path")
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>
>>>> Applies on top of v6.9-rc5. Passes all the mm selftests on arm64.
>>>>
>>>> Thanks,
>>>> Ryan
>>>>
>>>>
>>>> mm/huge_memory.c | 5 +++--
>>>> mm/pgtable-generic.c | 2 ++
>>>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 89f58c7603b2..80939ad00718 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2513,12 +2513,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>> * for this pmd), then we flush the SMP TLB and finally we write the
>>>> * non-huge version of the pmd entry with pmd_populate.
>>>> */
>>>> - old_pmd = pmdp_invalidate(vma, haddr, pmd);
>>>>
>>>> - pmd_migration = is_pmd_migration_entry(old_pmd);
>>>> + pmd_migration = is_pmd_migration_entry(*pmd);
>>>> if (unlikely(pmd_migration)) {
>>>> swp_entry_t entry;
>>>>
>>>> + old_pmd = *pmd;
>>>> entry = pmd_to_swp_entry(old_pmd);
>>>> page = pfn_swap_entry_to_page(entry);
>>>> write = is_writable_migration_entry(entry);
>>>> @@ -2529,6 +2529,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>> soft_dirty = pmd_swp_soft_dirty(old_pmd);
>>>> uffd_wp = pmd_swp_uffd_wp(old_pmd);
>>>> } else {
>>>> + old_pmd = pmdp_invalidate(vma, haddr, pmd);
>>>
>>> This looks good, except that now I am deeply confused about the pre-existing
>>> logic. I thought that migration entries were a subset of swap entries,
>>> but this code seems to be treating is_pmd_migration_entry() as a
>>> synonym for "is a swap entry". Can you shed any light on this for me?
>>
>> It is likely because kernel only split present pmd and migration pmd, but I
>> could be wrong since the code is changed a lot since splitting migration
>> pmd was added. We either need to check all call sites or check pmd_present()
>> instead of is_pmd_migration_entry() and handle all possible situations.
>>
>>>
>>>
>>>> page = pmd_page(old_pmd);
>>>> folio = page_folio(page);
>>>> if (pmd_dirty(old_pmd)) {
>>>> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
>>>> index 4fcd959dcc4d..74e34ea90656 100644
>>>> --- a/mm/pgtable-generic.c
>>>> +++ b/mm/pgtable-generic.c
>>>> @@ -198,6 +198,7 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
>>>> pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>>>> pmd_t *pmdp)
>>>> {
>>>> + VM_WARN_ON(!pmd_present(*pmdp));
>>>> pmd_t old = pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmdp));
>>>> flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
>>>> return old;
>>>> @@ -208,6 +209,7 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>>>> pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma, unsigned long address,
>>>> pmd_t *pmdp)
>>>> {
>>>> + VM_WARN_ON(!pmd_present(*pmdp));
>>>
>>> Should these be VM_WARN_ON_ONCE(), instead?
>>>
>>> Also, this seems like a good place to put a little comment in, to mark the
>>> new design constraint. Something like "Only present entries are allowed
>>> to be invalidated", perhaps.
>>>
>>>
>>>> return pmdp_invalidate(vma, address, pmdp);
>>>> }
>>>> #endif
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>
>>> thanks,
>>> --
>>> John Hubbard
>>> NVIDIA
>>
>>
>> --
>> Best Regards,
>> Yan, Zi
--
Best Regards,
Yan, Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
next prev parent reply other threads:[~2024-04-27 20:45 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 17:07 [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and GUP-fast Ryan Roberts
2024-04-25 18:58 ` Zi Yan
2024-04-26 4:50 ` Anshuman Khandual
2024-04-26 14:33 ` Zi Yan
2024-04-29 3:36 ` Anshuman Khandual
2024-04-26 7:48 ` Ryan Roberts
2024-04-26 4:19 ` Anshuman Khandual
2024-04-26 7:43 ` Ryan Roberts
2024-04-26 14:49 ` Zi Yan
2024-04-26 14:53 ` Zi Yan
2024-04-27 4:25 ` John Hubbard
2024-04-27 15:07 ` Zi Yan
2024-04-29 5:31 ` Anshuman Khandual
2024-04-29 5:25 ` Anshuman Khandual
2024-04-29 5:07 ` Anshuman Khandual
2024-04-27 4:41 ` John Hubbard
2024-04-27 15:14 ` Zi Yan
2024-04-27 19:11 ` John Hubbard
2024-04-27 20:45 ` Zi Yan [this message]
2024-04-27 20:48 ` Zi Yan
2024-04-29 6:17 ` Anshuman Khandual
2024-04-29 14:41 ` Zi Yan
2024-04-29 9:29 ` Ryan Roberts
2024-04-29 14:45 ` Zi Yan
2024-04-29 15:29 ` Zi Yan
2024-04-29 15:35 ` Ryan Roberts
2024-04-29 15:34 ` Ryan Roberts
2024-04-29 16:02 ` Zi Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E18F9D0B-C855-4252-BEEB-F8B1FA5BCE9E@nvidia.com \
--to=ziy@nvidia.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@kernel.org \
--cc=jhubbard@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=ryan.roberts@arm.com \
--cc=zi.yan@cs.rutgers.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).