Received-SPF: pass (google.com: domain of linux-kernel+bounces-160166-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Zi Yan <ziy@nvidia.com>
To: Anshuman Khandual <anshuman.khandual@arm.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>
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and
 GUP-fast
Date: Fri, 26 Apr 2024 10:33:30 -0400
Message-ID: <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com>
In-Reply-To: <af20fd17-1e31-4e28-8773-1298aa799aec@arm.com>
References: <20240425170704.3379492-1-ryan.roberts@arm.com>
 <922F6794-1C32-4862-8A67-90D4F7DBC474@nvidia.com>
 <af20fd17-1e31-4e28-8773-1298aa799aec@arm.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_57B41F34-0D4D-4C94-84B6-3A8737B633F1_=";
 micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?KFtWhcGjOFz7oTh9utLHZK6iyndbqG4Yemm7HKTZI4cUOi6yu/m8pIfmkUqp?=
 =?us-ascii?Q?mZt2lnPvzWL0CzBvcmWrMTyTDSiaGXjqibLbtgrdQIAZCyvby41GEmCpTI/I?=
 =?us-ascii?Q?HUgJHpnU5GMirOQk64kRbBrkUuBwkrCmVuMRWaQP8ySGe+Fi0fLkszu+no/R?=
 =?us-ascii?Q?cR4h6PwkTSNeBwR2XW55gX0o3+LaUqux5jSbwfn//ApxjEmiuB0cviu02ssO?=
 =?us-ascii?Q?WoSMXmcTv8pSzoew6xiS02UhExGOABHI+Bu4D7DGtOAvxDzw0Hq5F//kkiZg?=
 =?us-ascii?Q?zzum46qRzUyRweT69zGi/7IBUzilZsJnMSF5bgk7lFSO8nLinu+W1Wi6Rrbr?=
 =?us-ascii?Q?fXCsLUYOnUxF6ft1LS3ewmhVhjGVhN7TcOphDKAPONM50UDhbVZQDtUMo+Z5?=
 =?us-ascii?Q?kEBSbpGU8A8M72/6FWtq8z73iMbmffXzPDUtDSP4Fa5H0DxWlc1fWUH5BFTZ?=
 =?us-ascii?Q?TqXpZJgOiW5wA9WAKad2ejlzTz8XXDwx78+Mgu8Xk+nEKLfTWBWIa6LxJb7R?=
 =?us-ascii?Q?hRs3rTmZfzXILs4/9NfQ6D4sFQSJ2x8/ZDh4p4/vvAhSuVw5DVzfZw9Cgwwb?=
 =?us-ascii?Q?ER5ARPXvNwZTCx4lTCm+xlCsHczj6298O/KlrZbe69WCvJooAsBH01/UOTE+?=
 =?us-ascii?Q?AmthsKX0n8g1DRPRhX0bYtAh8oNuF1/JKVE3CXNed81KgyNZfadnR51ACIUL?=
 =?us-ascii?Q?d0sNSoB3DYiAyUzNKWofVk5ELpeR2AWJrBL19RR4xR+fGAWAy6P5LdJB9WFS?=
 =?us-ascii?Q?x12852dAyFanARG1Pl91lKBw7Ayp5R9avKYY1B3ijN9YH1wb3L9RSZqndRDT?=
 =?us-ascii?Q?jOpZ4bQ9BT3nmqXLomLJPjdLuTt61hulGGTZj2/uJPmOOq1lWIDEz02WJPZ9?=
 =?us-ascii?Q?Kq4wx/gwrGm8W3HzqHlDD5PRZzcfTJyy2kQBkM+1z+UKTGWZGL6zgp3fdmIB?=
 =?us-ascii?Q?TUClxMdURaxKK7+L/vGrhONCgOqyXLnVDodtThSn9XvOSP3jkS9LZdRvhxi9?=
 =?us-ascii?Q?1LxJ+t/L3/rc/5o9kR7QYZwOxmyhAlcay9Uv/F3CQSGwfW1ev5msM70mrsp1?=
 =?us-ascii?Q?j9Q7PJfMgW2zBZYUy7Rhj9IUgB/nryFixmbivVQ6Tx7oUVn6C7GmpdE/HiW0?=
 =?us-ascii?Q?SogTZt25i7xnOk2WU+WmmcvbcPQdAW6JPb2DKq+QDfx927Zxq0wozRrzl6Z6?=
 =?us-ascii?Q?CRA1VP7fbew7YZOC5OLrtRCOlbtcGRUxevYF2hRc1PJAFhKa8Qiphwzenp6V?=
 =?us-ascii?Q?oUvFC21puWm0irj91orjK0iWW4K+9o8nbiuJzQ2VAin4/4dRYnAz9/3Ka0TR?=
 =?us-ascii?Q?Zx/eBAI0X7c75lAJGyxvvqxZvAKXF40LYPJ0yIc+SRxUxGkQ/4DnT3uOEwt+?=
 =?us-ascii?Q?7lyHC3LZ/VjDtiQrVMIuOuX/hSsuOX5v95lRnZLCUe8uJX0rOKhdJ7NQT+CL?=
 =?us-ascii?Q?plmqAPyFzWrwxMpIRQhYVVuafTsfQUzLed2ho9w51GnTJ4WPmvHv6QZ08Wdj?=
 =?us-ascii?Q?/l7tOTyAzsRre46vR6FNb70/hedF3CmEtTl6m3QKuD6r8U2e7FduVYTs0kHS?=
 =?us-ascii?Q?SVneSRAAcg6ASG+w+xI=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 77a64f87-1a28-45df-7fee-08dc65fdd96b
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 14:33:32.9802
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: I0zJKG9p47B8MQIaWi7SKnSZY/e7mUCG7KOwZzF1cuQZTn/laY5ePFzKDIF6X4kh
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR12MB5622

--=_MailMate_57B41F34-0D4D-4C94-84B6-3A8737B633F1_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


--
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=
=2Ekhandual@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 n=
ot
>>> based on the returned old pmd. This is a problem for the migration en=
try
>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must only b=
e
>>> 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 th=
e
> PMD_SECT_VALID, thus erasing its identity as a huge mapping. A software=

> bit PMD_PRESENT_INVALID was added which helps in preserving that cleare=
d
> huge page mapping identity once it becomes unmapped.

OK. PMD_SECT_VALID is just a different name of PTE_VALID. I wonder
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.

>>
>>> lockless pgtable walker could see the migration entry pmd in this sta=
te
>>> and start interpretting the fields as if it were present, leading to
>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walk=
er.
>>> 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 pa=
th")
>>> 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 th=
e
>>>  	 * non-huge version of the pmd entry with pmd_populate.
>>>  	 */
>>> -	old_pmd =3D pmdp_invalidate(vma, haddr, pmd);
>>>
>>> -	pmd_migration =3D is_pmd_migration_entry(old_pmd);
>>> +	pmd_migration =3D is_pmd_migration_entry(*pmd);
>>>  	if (unlikely(pmd_migration)) {
>>>  		swp_entry_t entry;
>>>
>>> +		old_pmd =3D *pmd;
>>>  		entry =3D pmd_to_swp_entry(old_pmd);
>>>  		page =3D pfn_swap_entry_to_page(entry);
>>>  		write =3D is_writable_migration_entry(entry);
>>> @@ -2529,6 +2529,7 @@ static void __split_huge_pmd_locked(struct vm_a=
rea_struct *vma, pmd_t *pmd,
>>>  		soft_dirty =3D pmd_swp_soft_dirty(old_pmd);
>>>  		uffd_wp =3D pmd_swp_uffd_wp(old_pmd);
>>>  	} else {
>>> +		old_pmd =3D pmdp_invalidate(vma, haddr, pmd);
>>>  		page =3D pmd_page(old_pmd);
>>>  		folio =3D 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_s=
truct *mm, pmd_t *pmdp)
>>>  pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long addr=
ess,
>>>  		     pmd_t *pmdp)
>>>  {
>>> +	VM_WARN_ON(!pmd_present(*pmdp));
>>>  	pmd_t old =3D pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmd=
p));
>>>  	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 a=
ddress,
>>>  			 pmd_t *pmdp)
>>>  {
>>> +	VM_WARN_ON(!pmd_present(*pmdp));  	return pmdp_invalidate(vma, addr=
ess, pmdp);
>>>  }
>>>  #endif
>>> --
>>> 2.25.1
>>
>>
>> --
>> Best Regards,
>> Yan, Zi
--=_MailMate_57B41F34-0D4D-4C94-84B6-3A8737B633F1_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYruzoPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhU7PoP/jfs5YKnXYr2r55OVOhV+J1mfXbHWK2S+tUn
Q9gWwOmCQhXX4QJQ2rD/Nc/48ypGhhMQO296X36sz8vufT50AWowWQ2s88f7iQ+m
GjmAHr025m0yqR8c/hDFxrGz1AFBV2yz6AEzsQRJSdisJKnehu0zCHIy+BLZ9cxE
B7pl91hsgQlUgc218/nVqLZv3uxakwEcSNCpW5p8+agok6IF8GyzXAMZxYhcAwgq
RXVH9TEAixWO4QFtxigNfNCppubVI/BEs8R+i/ksfMWjCXf2kXWDEGajsZt7CNlz
fxV4QZJZ6RvbrXuNzNt2HcfYG6boU8Jx/Y5pT7s8joTedyc0tf5w1qCRhN9aEuil
Y08MMh5YFGBG8cGF1sEvGn1K+un2YMsaHdXpMxJvVPCz3XBeCWbcP3hei6OVVVER
p6KIKTFi7CtOGrItrtw7vpiFSmKo/1hiV+lAKQb6F/sHceuEEL0e7iRS7Bq81zng
mbPzlf/0bXSsKnZCr1QYYXPXN59kfRJkramIdtFLoWQKLU797J0nqRMcYAld5Sf1
fzGSGoAq9o+0j/Xclrvjd1+edZZoP3NqjlMk1846d7fQNnRulW93NzzgkHFKpwJg
s548SR8OXzOhhYbn+eHjUlNwwWceuUcdPjM/cB8AypvPhw7hslAEy1YR2cGFYSoa
72q9Dd94
=oulT
-----END PGP SIGNATURE-----

--=_MailMate_57B41F34-0D4D-4C94-84B6-3A8737B633F1_=--