Received-SPF: pass (google.com: domain of linux-kernel+bounces-161699-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <34a9ec83-40ab-4028-8ee3-27438665155f@arm.com>
Date: Mon, 29 Apr 2024 09:06:22 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and
 GUP-fast
Content-Language: en-US
To: Zi Yan <ziy@nvidia.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, Huacai Chen <chenhuacai@kernel.org>
References: <20240425170704.3379492-1-ryan.roberts@arm.com>
 <922F6794-1C32-4862-8A67-90D4F7DBC474@nvidia.com>
 <af20fd17-1e31-4e28-8773-1298aa799aec@arm.com>
 <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com>
From: Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 4/26/24 20:03, Zi Yan wrote:
> 
> 
> --
> Best Regards,
> Yan, Zi
> 
> On 26 Apr 2024, at 0:50, Anshuman Khandual wrote:
> 
>> On 4/26/24 00:28, Zi Yan wrote:
>>> +Anshuman, who changed pmd_present() semantics. See:
>>> https://lore.kernel.org/all/1599627183-14453-2-git-send-email-anshuman.khandual@arm.com/ and commit b65399f6111b ("arm64/mm: Change
>>> THP helpers to comply with generic MM semantics")
>>>
>>> On 25 Apr 2024, at 13:07, 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
>>>
>>> But pmd_mkinvalid() on x86 does not behave so. Maybe we should fix
>>> pmd_mkinvalid() on arm64 by not setting PMD_PRESENT_INVALID when the
>>> entry is invalid already. And add a test in mm/debug_vm_pgtable.c.
>>>
>>> I notice that x86, risc-v, mips behave the same. loongarch also
>>> has _PAGE_PRESENT_INVALID bit set during pmd_mkinvalid(), but its
>>> pmd_present() makes sure _PAGE_HUEG is set before checks _PAGE_PRESENT_INVALID.
>>> So it is not a problem for loongarch. Add Huacai to confirm this.
>>>
>>> Maybe pmd_present() on arm64 can do that too?
>>
>> pmd_present() should return true even for a splitting PMD which is not
>> currently mapped. IIRC in all other architectures, there is a distinct
>> identification bit for huge page which stays back, even when the entry
>> becomes unmapped. That bit helps pmd_present() return true, during PMD
>> splitting process.
>>
>> But on arm64 platform
>>
>> #define PTE_VALID                (_AT(pteval_t, 1) << 0)
>> #define PMD_SECT_VALID           (_AT(pmdval_t, 1) << 0)
>> #define PMD_TABLE_BIT            (_AT(pmdval_t, 1) << 1)
>>
>> When the PMD entry becomes unmapped, PTE_VALID gets cleared, so does the
>> PMD_SECT_VALID, thus erasing its identity as a huge mapping. A software
>> bit PMD_PRESENT_INVALID was added which helps in preserving that cleared
>> huge page mapping identity once it becomes unmapped.
> 
> OK. PMD_SECT_VALID is just a different name of PTE_VALID. I wonder

Non PTE Level descriptor

	BIT[1:0] = X0 Invalid entry
		 = 01 Block entries (Huge Pages)
		 = 11 Table entries

PTE Level descriptor

	BIT[1:0] = X0 Invalid entry
		 = 01 Reserved invalid entry
		 = 11 Page entries

Although PTE_VALID and PMD_SECT_VALID share the same bit position, the huge
page also requires the table bit to be cleared.

> if ~PMD_TABLE_BIT can be used as _PAGE_HUGE to indicate it is a huge page
> PMD, since PMD_TABLE_BIT is unset for PMD huge page already, for swap
> entry, since PMD_SECT_VALID is unset, PMD_TABLE_BIT is ignored. But it
> will require PTE and PMD have different swap entry encoding on arm64.
> It might not be worth the effort.

Right and also depending on just clearing of table bit for huge page mapping
might be problematic in subtle ways.

> 
>>>
>>>> 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
>>>> 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);
>>>>  		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));  	return pmdp_invalidate(vma, address, pmdp);
>>>>  }
>>>>  #endif
>>>> --
>>>> 2.25.1
>>>
>>>
>>> --
>>> Best Regards,
>>> Yan, Zi