Received-SPF: pass (google.com: domain of linux-kernel+bounces-162469-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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 10:45:22 -0400
Message-ID: <64037B35-3E80-4367-BA0B-334356373A78@nvidia.com>
In-Reply-To: <23fbca83-a175-4d5a-8cf7-ed54c1830d37@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>
Content-Type: multipart/signed;
 boundary="=_MailMate_23555A3C-255D-4C75-9BDF-7BCBE4832D88_=";
 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?RGhpVVpORm9DQ05Ga2tPQW9ZTndaSzgvSzh0d0hQdFNWekxnRDIzczdWY2lu?=
 =?utf-8?B?QnRrNFdBbjJVemhmb0t5M3Q0R1pMQVhwSGRONXNDYTBuMUhiOUsyVGUxcElt?=
 =?utf-8?B?RVBXbDJPTTJzeEJYUTVDZkp1MUNjSzdFL24zYVhKY1p1TWJGL05xRU5iU1Ex?=
 =?utf-8?B?TkpnUGhFbWhmK3FEN3cyWUpNRktOWnJIRTJSVVg4dk9na3lSWStJaTFCQ21E?=
 =?utf-8?B?bGk1a3pXK0w2MXpZZUZsWEwxZkFtYVlhTDJFNnFUVDNFUVF1WFB3R3lrWm1m?=
 =?utf-8?B?ZjdHdFFhcXJPeDhIRW5lVFlmb1BpbmovdEk3TXc0cEQzS0hyME16cFRHL1dX?=
 =?utf-8?B?dHZIWUFxUXR2SnJLZk85NlhLREc3cVoxZmlmbGlKUUplL0ZTUkFuRDJUZGpU?=
 =?utf-8?B?OGhXWkwxWVptOU1KM2t4bXRrQUdxSnZMSGhlNHJqU01heEZHZ2xqM2ZLcVB3?=
 =?utf-8?B?NmJUcFRucEtLakhEM0dBdUk5ODdpbnpjMGswVmxvZ2VSZ2hxRDBQUERkYnlq?=
 =?utf-8?B?S3JyZUt3SGZPc2Jyb1NQSUllNFNhdUo4QzFpSVNnM0Vtd0ozNTY5VlJLQVB1?=
 =?utf-8?B?NThyNS8rRlkxRGl0S0JlMHBNZlh4U1R3eW9CU0Q5OFhSU1BJWFlMMDlqenNX?=
 =?utf-8?B?bU4zeDBwSCt0WDJ0aHVIN0MybUgvVDBFTHZKeXNVTjRYeUlOdlhFc2VaRisr?=
 =?utf-8?B?czd5TE8wKy9CRDg2cGJpaXhzb2JZc2dHYWJlSzZOS3pmYW9WVXNBZXVNMXZo?=
 =?utf-8?B?SWVSL0JEK1dXL1E1ZWZiejYvdmlLNnc0cFlpUG5HSUpGQzNTeVU3YW9vMk9T?=
 =?utf-8?B?c3orVXVyZ0QyY1lWVzcyeHpzMk5hS1RTMGxUZnhiM0VnYVlEUUszSGllM3Iw?=
 =?utf-8?B?Wmc1cFR5dm83amk5cXQ5dklHL0lKT1d1TUJ6cGNNaFRWS2NjQS9DSS9WSzU3?=
 =?utf-8?B?M3NucC9SVGJBdS90UnRzcHlTZzVhYVRtUmtTRko0Y2NLUzc1b2VvcUErRTNK?=
 =?utf-8?B?U0hJZUQyQm5ZeWdsUVdBKzM2NFNBc0J4YXl6bXV4dmJWd2JSWGVHcVo4NEcz?=
 =?utf-8?B?RmxRMkJtOUUrNnVDNG53M2J3Z0taamtlak9uV0x0VytZZTZJSlJxUDU1VjhQ?=
 =?utf-8?B?ZW1QdnpIRFhtcDh5MTUrOW16SVlyVjkrK2JGRzlObktpVmR5b041d2h2TURE?=
 =?utf-8?B?bG9oMlNPZFFXRGhkdjZIM3JGV2NsVU1Ka1N0by9HakJzSFE4UXlqZnZJWXZm?=
 =?utf-8?B?Q0ZaVUdKMVczakUvbzIzSWFKaWorWnk0dUFjQmIyQVZrUzRqR0JyRERxbDB1?=
 =?utf-8?B?WDRseDUwWGhNNkNWZWhXMjNkc0wzb0RpR1VSR3dvbnUzcXR0THQ4OUdQaEdh?=
 =?utf-8?B?emdscVhJNm1vZ1pUc2lPZC9NenJNekNPenNiZzdBNzREbzE2L3lCcHB2UGhh?=
 =?utf-8?B?MENNVmJsekVwTWdPejFBRHdFdzRWSnJYaUNPQkdzMGovbFhzZXVxbTVPb0hQ?=
 =?utf-8?B?TktqSGxjckMrU2hreFREU0F0Z2pLYzVrYTZsMFhPdVNOb09SNmtoNzBGNjEx?=
 =?utf-8?B?Zm9xb2dCVHkvbXo1cUZERjZGbTJMMGlaMUFONEtBeEZzcHdkMTRhb1NaaVUr?=
 =?utf-8?B?TWtpSDNoVHJwWGN1R2tDWDRTQXJwTlU4WXRHQXpMYmR0bWJ1RXd4M1dtUUtF?=
 =?utf-8?B?QUNsYjNKTUR0QzJVUDBCdk5xa01YQS90K0d4eFA2YVVFSjBuWkd0Yy9sUVM4?=
 =?utf-8?B?Z1Z2c1lOMFJmN2paazQ3MklXOE9YMmxmaW9RV0NzcC9mUWVlY2kyQUxuMk1S?=
 =?utf-8?B?VGZSRS90NUNvOTN5QXRVd1djZWozcmJ4dER1UW9iQzZBalF3OVJYNGpYTk9x?=
 =?utf-8?B?SEd0TnZuOEh1STdwQUVvcGRWUUZHLzZWYlgzcUwzOXZYdUw1UDJOYytFdWFv?=
 =?utf-8?B?ZTVNVjJ2OUd3ZXZkK0N3MjBlU3NvRU96bUNXaEg1TmQ0REg3ak0yaHgyWERt?=
 =?utf-8?B?ZjlhS2pNMWFQRmE3ZjloYnhFOWdPWHN2UFZpVzJHa2pBOXZ1UFpPR1h1UUtV?=
 =?utf-8?B?cGlPOUJIK3YybVBYQzk0TGQvS0kxWkJzN2Nicm4xcFhRQ1QzaU9YTUtiZTUz?=
 =?utf-8?Q?Jcud4MYWQTcnjrV/ZzOlkQZt0?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6558a381-9a49-49bb-78b3-08dc685b00ce
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 14:45:24.5553
 (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: T3wMjrPnm22z9H85U1YnKnsq3MA1a8u10GS02EOm+cwlTp03gD+b/Ey1YObspqJY
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB7173

--=_MailMate_23555A3C-255D-4C75-9BDF-7BCBE4832D88_=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 o=
r
>>>>> (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 s=
tate
>>>>> and start interpretting the fields as if it were present, leading t=
o
>>>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable wa=
lker.
>>>>> I suspect the same is possible on other architectures.
>>>>>
>>>>> Fix this by only calling pmdp_invalidate() for a present pmd. And f=
or
>>>>
>>>> Yes, this seems like a good design decision (after reading through t=
he
>>>> discussion that you all had in the other threads).
>>>
>>> This will only be good for arm64 and does not prevent other arch deve=
lopers
>>> 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:
>
> Hi All,
>
> Thanks for all the feedback! I had thought that this patch would be ent=
irely
> uncontraversial - obviously I was wrong :)
>
> I've read all the emails, and trying to summarize a way forward here...=

>
>>
>> 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 enfor=
cement
>> =C2=A0=C2=A0 we can provide, in order to keep future code out of troub=
le.
>
> I know its only semantics, but I don't view this as a new design constr=
aint. I
> see it as an existing constraint that was previously being violated, an=
d this
> patch aims to fix that. The generic version of pmdp_invalidate() uncond=
itionally
> does a tlb invalidation on the address range covered by the pmd. That m=
akes no
> sense unless the pmd was previously present. So my conclusion is that t=
he
> function only expects to be called for present pmds.
>
> Additionally Documentation/mm/arch_pgtable_helpers.rst already says thi=
s:
>
> "
> | 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, if =
it helps?
>
> Finally, [1] which is linked from Documentation/mm/arch_pgtable_helpers=
=2Erst,
> also implies this constraint, although it doesn't explicitly say it.
>
> [1] https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.com/=

>
>>
>> 3.a) See the VM_WARN_ON() hunks below.
>
> It sounds like everybody would be happy if I sprinkle these into the ar=
ches that
> override pmdp_invalidate[_ad]()? There are 3 arches that have their own=
 version
> of pmdp_invalidate(); powerpc, s390 and sparc. And 1 that has its own v=
ersion 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 se=
t a
> precedent for adding them absolutely everywhere. (e.g. pte_mkdirty(), .=
=2E.).

I understand your concern here. I assume you also understand the potentia=
l issue
with this, namely it does not prevent one from using pmd_mkinvalid() impr=
operly
and causing a bug and the bug might only appear on arm64.

>
>>
>> 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 no=
n-present page
>> =C2=A0=C2=A0=C2=A0=C2=A0 table entry.
>>
>> I do wonder if there is somewhere else that this should be documented?=

>
> If I change:
>
> "
> | pmd_mkinvalid             | Invalidates a mapped PMD [1]             =
        |
> "
>
> To:
>
> "
> | pmd_mkinvalid             | Invalidates a present PMD; do not call fo=
r       |
> |                             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.

--
Best Regards,
Yan, Zi

--=_MailMate_23555A3C-255D-4C75-9BDF-7BCBE4832D88_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYvsoIPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUnOAQAJILpwZal3+OXf+nlVlsToUHSW0w59IizgWO
EW6ZVuwOmrkVciIG2VyBGy56sAVXjS2Hpwm3eoLYWQg56C8j3SyAM6GqM4FplxfT
aJwejSLCB5es+XQ3YdmUZvuZodSfgJyGfxreqI4uJFnqROVAvXrcmvc9saMERS17
XDajRW1QUkr/gpVDvb4sjO+EeG7l37X0zRRN6khBWIXd8zdpzWjpjd3vSO9pePXm
o/uI70Urhsydqm2pzW4Aw/t8bBr8tj5gBkQkARnMk7l0vXoFR3UmL7j+lM0w6xlC
fKGBvryqwIe+5B4Ed27ESuxUkkBQk5vToirw9sEHehjeonkOOFj7UeCsiEHHrHFy
cAy+pajdJ4NSf8lYMDl7Ex+TiU/Mdspqq6hUN2XpdbEKrU9jkXtzPucbPBjddReQ
2n/f2oEiDKVTIDZE/XcKVosbPHfHCbfmsDbFAY1GRHXOvhJcEH1245cRQK9T4NDT
IaLkVz1tPh06Q43Xy0iJtPuzRz3HEHtJF3TRfIUsp3zF0ZKnQ7hV0rddzr2th+PO
MpZ/PGBAk+Z0GxARLI1ZpgDs7PoqTUAvDrCrJ2ksamEwx8cDhwfz6HSHElIHL+iP
CCO+SMRbrrJ6J7f5tyo0D1GxRyttOP1tA8iDE3Uvpk5E1q2Isx1tIWRtNfl1j6yL
xJsg6dKc
=GMop
-----END PGP SIGNATURE-----

--=_MailMate_23555A3C-255D-4C75-9BDF-7BCBE4832D88_=--