Received-SPF: pass (google.com: domain of linux-kernel+bounces-162645-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Zi Yan <ziy@nvidia.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: John Hubbard <jhubbard@nvidia.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: Mon, 29 Apr 2024 12:02:59 -0400
Message-ID: <FF2DE3E1-0A11-4DAC-9176-969B7DCAE3A1@nvidia.com>
In-Reply-To: <99384a25-9ff5-43c9-b09d-5a048c456d02@arm.com>
References: <20240425170704.3379492-1-ryan.roberts@arm.com>
 <17956e0f-1101-42d7-9cba-87e196312484@nvidia.com>
 <2C9D897B-0E96-42F0-B3C4-9926E6DF5F4B@nvidia.com>
 <328b2b69-e4ab-4a00-9137-1f7c3dc2d195@nvidia.com>
 <23fbca83-a175-4d5a-8cf7-ed54c1830d37@arm.com>
 <64037B35-3E80-4367-BA0B-334356373A78@nvidia.com>
 <99384a25-9ff5-43c9-b09d-5a048c456d02@arm.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_CEEAA46B-5331-4857-9DF4-263C6FE31C93_=";
 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:
	=?utf-8?B?ZEdoS01PSGhISkxwR0V2VnVLN21YengxcXhDU1BIOFl4RFJiUDVUZXNJWmxF?=
 =?utf-8?B?SlYxQlh2Ni9JN3cxL0NnVmdQSDJYangxTHArTTRTTE5WbEJvL1R3Rkd5b21J?=
 =?utf-8?B?YlBrVzVaT1Jlb1NKdTNIckZISmhsNTAwaDZoZytnRzdzb3hsaUVNV0pJWmFt?=
 =?utf-8?B?K0hHYzRPWkw2R2gyTkRNYjkwZFdRWlFsMTNEc2sxc1lwVjFObTVsalZ3Q1dU?=
 =?utf-8?B?enhUR3B0eXZRUUY1N0JaV1EyeXV4MDRhdlhrYU5rNWlybE0zV281NFlJSWE5?=
 =?utf-8?B?UVZJZTV0UFlNTE9hQ0xST3I5VVIwOW9la2c0THYwY0xxckZ2bmdxV0d3V2ZG?=
 =?utf-8?B?S1NJT2NuTk8yYVR1ZmFLU1NJVjBEaS9mUmtQSEdWMXkzSS9NNncybnBqTWxN?=
 =?utf-8?B?TFpxdkw4L0FqMDFuSFMzczVISFNmL3o3bEQvcEVxZER0Rzk4UHpVNzVCTjJW?=
 =?utf-8?B?T2RGc3N2cFJvYkRSODBoYmtCaTRTejNyOFNIbEdqcUZJZWpWZS9LWEtBTmIv?=
 =?utf-8?B?NEpEUElJQkJld1dIb0trYmZuUFJiNVlVVy8raDc5QzlLL1gwQjBMS2hrZkFz?=
 =?utf-8?B?TStKc3FEYnQ2RmFmZzloL2IyNkxBQWVKWEJPWDd1dmlUL2lSM25NdFE0dkVz?=
 =?utf-8?B?eWdsRGxwUTFudjhFSVBwdFpMN0dDWWZ3eHFlbUthbXBEMk1semtaWU5xSnNi?=
 =?utf-8?B?bEZRNmtScklrZENtYXdUV1lzdE9BZFVsaVJuMkVJUUI0T0kwU01pUWovL1BZ?=
 =?utf-8?B?N2pwNjMwTkFtWTFmR0wrL3ZSeDBmZ0dwZ1pKbXBFV09Vam5TTzMrWXRaZkpJ?=
 =?utf-8?B?bHdaR0pIek16RmFDNy9vditvZmk5VWxUQkJJSGlyR2x3TmhsRHAyK2thaW5P?=
 =?utf-8?B?cmRIZVd2Vm5RUHpndDd3RTlQcUtyL0NMRHZsdTltYmlwUm9rZnZrSDhXSzRn?=
 =?utf-8?B?YmpmRW95R1llR2VURXhPL01iSlY4QlhYZzFPakxoY1o1dlVmOWdZZy9ndGtX?=
 =?utf-8?B?VngxdmE3TXR3VlUzdnNlS1Z4dlNxRlNycTJ4SG9CTlU2VkQrcVRkR0lHNFNQ?=
 =?utf-8?B?VlNuOXJwK2U4NUZzNk54cHQzNEhvWWg2aXZMVE5aK2treHJRNlJISHM2ZmRP?=
 =?utf-8?B?eXgzZFlVamhWRjRDOVJ0QXlPU1IvbVROVTdwV0hWcThTSmdvTFhwVEpHOG54?=
 =?utf-8?B?UVZLenlSanJ2RkxoT1ZqTlUyYXRwWGFKRitUZWx1VGhkeTFTbDE5RmdNUkg5?=
 =?utf-8?B?N0J0UXlseCtyRmM5RDhJSmQ5aS9GamkzV1VVRDdlMzg3WHVORzBlOTd0Mzl3?=
 =?utf-8?B?ZkZ5OG10NW1wb1FCaDFOTVZXczN1T1kzNGU1bWprUGpNV2d1Wk8zQzZvckFi?=
 =?utf-8?B?ZUQzZVAzOFlVYm12S3VFTUpUaG1WbzdpQm1oZWtEY2d0aDRxeDFCZUhoam9E?=
 =?utf-8?B?MWwrUDFiRjJjejBPQnlOdzlOSUtPU3pSWnFZYWpuVjYyUEQ2NXFEcE0rOEFH?=
 =?utf-8?B?UFFHYWV6Y0J3VURLdHNnY3BPNm53bEVxOTI5NTFjWHRlQ2N2ekJ2Smt5aGQ3?=
 =?utf-8?B?Uzd0a0VwZnRjK1JmOUxHVkJVK0tURS9PSE5QK1pra0xPaXdjNFVMazNidHNh?=
 =?utf-8?B?Tmk2MnZYMEVPQjA2LzFqVk4xRVFQSXR0YUZnMitpN1U4UGJkclgydGFzbkYw?=
 =?utf-8?B?d25NWFhVUWRrdThhQ2NrSGpJNFJLZFBjM0RuWnVESDlrdEdZNXhOUGd1MXFK?=
 =?utf-8?B?cGttcTZFN0MvVUo2YytFNFNzNFBSMFdJa05hU1NXUXJ4UWdBeVF0ZkVGOG41?=
 =?utf-8?B?bDdsZmNVQ01uUEY4MytheHo1RC93SnVrc2daVVAwcmFGN2RZY0VDb0ttRyt3?=
 =?utf-8?B?VWtrZTFDSUFqbHp6WmNhMTkzSFhrWTRXQkdnY2JLRjlSQlFFZy9mdzB2R1BX?=
 =?utf-8?B?N0R3TzZOVGM5Q0E5aXJYeCtlcEN5R1RwRUtHZnRPSVUyRGFvZFlvM0lZSCt0?=
 =?utf-8?B?ZHpKRXFEVW80UGZQOWxGTXZUSUVDcUtidHlsQW5pTXQ1bUNBdWZXYUlQWUdz?=
 =?utf-8?B?RlRsRmZVOWxINW55Wk5zUHY1STh0dVBiZEJzRmJtU1R3MHV6eFBHWFJ5ZGQ4?=
 =?utf-8?Q?uiLRWPlpun0edsGVdZCmYUcO2?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b2216dc-8a89-42b8-7070-08dc6865d8c7
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 16:03:01.9628
 (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: kco58AuorVnU2j2grwyjirz7xmSmH8SN4r+UHOhGXmOhKo/eoYNRXzCSmqCnyYVi
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB9008

--=_MailMate_CEEAA46B-5331-4857-9DF4-263C6FE31C93_=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 29 Apr 2024, at 11:34, Ryan Roberts wrote:

> On 29/04/2024 15:45, Zi Yan wrote:
>> On 29 Apr 2024, at 5:29, Ryan Roberts wrote:
>>
>>> On 27/04/2024 20: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 migratio=
n entry
>>>>>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must on=
ly be
>>>>>>> called for a present pmd.
>>>>>>>
>>>>>>> On arm64 at least, pmd_mkinvalid() will mark the pmd such that an=
y
>>>>>>> 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 de=
velopers
>>>>> to write code breaking arm64, since only arm64's pmd_mkinvalid() ca=
n turn
>>>>> a swap entry to a pmd_present() entry.
>>>>
>>>> Well, let's characterize it in a bit more detail, then:
>>>
>>> Hi All,
>>>
>>> Thanks for all the feedback! I had thought that this patch would be e=
ntirely
>>> uncontraversial - obviously I was wrong :)
>>>
>>> I've read all the emails, and trying to summarize a way forward here.=
=2E.
>>>
>>>>
>>>> 1) This patch will make things better for arm64. That's important!
>>>>
>>>> 2) Equally important, this patch does not make anything worse for
>>>> =C2=A0=C2=A0 other CPU arches.
>>>>
>>>> 3) This patch represents a new design constraint on the CPU arch
>>>> =C2=A0=C2=A0 layer, and thus requires documentation and whatever enf=
orcement
>>>> =C2=A0=C2=A0 we can provide, in order to keep future code out of tro=
uble.
>>>
>>> I know its only semantics, but I don't view this as a new design cons=
traint. I
>>> see it as an existing constraint that was previously being violated, =
and this
>>> patch aims to fix that. The generic version of pmdp_invalidate() unco=
nditionally
>>> does a tlb invalidation on the address range covered by the pmd. That=
 makes no
>>> sense unless the pmd was previously present. So my conclusion is that=
 the
>>> function only expects to be called for present pmds.
>>>
>>> Additionally Documentation/mm/arch_pgtable_helpers.rst already says t=
his:
>>>
>>> "
>>> | pmd_mkinvalid             | Invalidates a mapped PMD [1]           =
          |
>>> "
>>>
>>> I read "mapped" to be a synonym for "present". So I think its already=

>>> documented. Happy to explcitly change "mapped" to "present" though, i=
f it helps?
>>>
>>> Finally, [1] which is linked from Documentation/mm/arch_pgtable_helpe=
rs.rst,
>>> also implies this constraint, although it doesn't explicitly say it.
>>>
>>> [1] https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.co=
m/
>>>
>>>>
>>>> 3.a) See the VM_WARN_ON() hunks below.
>>>
>>> It sounds like everybody would be happy if I sprinkle these into the =
arches that
>>> override pmdp_invalidate[_ad]()? There are 3 arches that have their o=
wn version
>>> of pmdp_invalidate(); powerpc, s390 and sparc. And 1 that has its own=
 version of
>>> pmdp_invalidate_ad(); x86. I'll add them in all of those.
>>>
>>> I'll use VM_WARN_ON_ONCE() as suggested by John.
>>>
>>> I'd rather not put it directly into pmd_mkinvalid() since that would =
set a
>>> precedent for adding them absolutely everywhere. (e.g. pte_mkdirty(),=
 ...).
>>
>> I understand your concern here. I assume you also understand the poten=
tial issue
>> with this, namely it does not prevent one from using pmd_mkinvalid() i=
mproperly
>> and causing a bug and the bug might only appear on arm64.
>
> Are you saying that arm64 is the *only* arch where pmd_mkinvalid() can =
turn a
> swap pte into a present pte? I hadn't appreciated that; in your first r=
eply to

Yes.

> this patch you said "I notice that x86, risc-v, mips behave the same" -=
 I
> thought you were saying they behaved the same as arm64, but on re-readi=
ng, I
> think I've taken that out of context.
>
> But in spite of that, it still remains my view that making arm64's
> pmd_mkinvalid() robust to non-present ptes is not the right fix - or at=
 least
> not sufficient on its own. That change on its own would still result in=
 issuing
> a TLBI for the non-present pte from pmdp_invalidate(). That's not a cor=
rectness
> issue, but certainly could be a performance concern.

I agree with you that using pmd_mkinvalid() on non-presenet entries does =
not make
sense, but there is no easy way of enforcing it to prevent anyone doing t=
hat. And
if people do it and they are not working or testing on arm64, they can br=
eak arm64
without noticing it. It becomes arm64's burden to watch out for this pote=
ntial
break all the time.

Yes, TLB invalidation should be avoided in pmdp_invalidate() to recover p=
erformance
loss. It is a separate issue from the pmd_mkinvalid() correction issue. T=
hank you
for pointing this out explicitly.

>
> I think its much better to have the design constraint that pmd_mkinvali=
d(),
> pmdp_invalidate() and pmdp_invalidate_ad() can only be called for prese=
nt ptes.
> And I think the combination of WARNs and docs that we've discussed shou=
ld be
> enough to allay your concerns about introduction of arm64-specific bugs=
=2E

Yes. I also understand that putting a WARN in pmd_mkinvalid() might not b=
e desirable.

>
>>
>>>
>>>>
>>>> 3.b) I like the new design constraint, because it is reasonable and
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 clearly understandable: don't invalidate a =
non-present page
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 table entry.
>>>>
>>>> I do wonder if there is somewhere else that this should be documente=
d?
>>>
>>> If I change:
>>>
>>> "
>>> | pmd_mkinvalid             | Invalidates a mapped PMD [1]           =
          |
>>> "
>>>
>>> To:
>>>
>>> "
>>> | pmd_mkinvalid             | Invalidates a present PMD; do not call =
for       |
>>> |                             non-present pmd [1]                    =
          |
>>> "
>>>
>>> Is that sufficient? (I'll do the same for pud_mkinvalid() too.
>>
>> Sounds good to me.
>>
>> Also, if you move pmdp_invalidate(), please move the big comment with =
it to
>> avoid confusion. Thanks.
>
> Yes good spot, I'll move it.
>
>>
>> --
>> Best Regards,
>> Yan, Zi


--
Best Regards,
Yan, Zi

--=_MailMate_CEEAA46B-5331-4857-9DF4-263C6FE31C93_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYvxLMPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUZkgQAKPohuRh2FC1Jjftnh86DhRQy04nN5Ygobeq
fkNI7ztVfZIJjGG9AORdK4R1pqEW2hqguZnCPjjr++a7fqMvgxmozNEHrRaa1ER8
46ijqjkCcX5Jm5hi9mNgqHO59grQBRliEKiIyTRksrPLQ5iKmjzKf/p6bCCMvME5
tyQuB5lK0JbingO5HWyo2JMXJbjIv5GSHsfZrNHCx7c1KHMHWYOd8q4YT1o/SgJf
EJcx0ShvmrKP/j3R25Yn0vnhDmx6BC1Gm3LIZ0Dh6B7cj/v+x5AbXYkLzgI4FzwO
LTqU2P0cCGjsjGEqaUXqYYLS/x0qy5cenWno66DrhDfJx3ioc/6rGhymXqIlbCkF
JFedflyiy4YZwthRGV5tFtt6GnxYJAE8Dp8W2xYJj7akRtn4W/8pkjlv4S0Zg4I4
IWT9Y8reSvobfIsOk3Jr+k4p4TH8onILhslsGMt5YIzRGs1mwSBxaYIq8gid484J
BnwRKYYJ20bdCvDgapYLScaw3gQHuIfpohuuhHrlsLHDTvX7uGf9UCvpk3+OmhU0
fj+T9kAKYzjkevUwHyUyZ3u+kLg7j2cdT4Gog5hlSbgykxwMEUJKDAl00v4OxNu8
AiJwFu7+nmc/26NcYzlxg9sIRPlJUn4bkO14OofQJnB+oDXk+/jTpHkSlknaIEFX
g3jFX+AL
=lLXt
-----END PGP SIGNATURE-----

--=_MailMate_CEEAA46B-5331-4857-9DF4-263C6FE31C93_=--