Received-SPF: pass (google.com: domain of linux-kernel+bounces-166495-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: Andrew Morton <akpm@linux-foundation.org>,
 Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>,
 Mark Rutland <mark.rutland@arm.com>,
 Anshuman Khandual <anshuman.khandual@arm.com>,
 "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>,
 Jonathan Corbet <corbet@lwn.net>, Nicholas Piggin <npiggin@gmail.com>,
 Christophe Leroy <christophe.leroy@csgroup.eu>,
 "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
 Christian Borntraeger <borntraeger@linux.ibm.com>,
 Sven Schnelle <svens@linux.ibm.com>, "David S. Miller" <davem@davemloft.net>,
 Andreas Larsson <andreas@gaisler.com>,
 Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>,
 Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>,
 Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
 linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v3] mm: Fix race between __split_huge_pmd_locked() and
 GUP-fast
Date: Thu, 02 May 2024 09:16:55 -0400
Message-ID: <EC1125D5-EED9-4D03-BCC1-77BB06262A1A@nvidia.com>
In-Reply-To: <2f95dac3-51cc-474d-8ef0-7af9dd09784b@arm.com>
References: <20240501143310.1381675-1-ryan.roberts@arm.com>
 <4992C22D-D7B7-4B6F-8A50-E084163BEF42@nvidia.com>
 <2f95dac3-51cc-474d-8ef0-7af9dd09784b@arm.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_38CF2DC2-489D-4DC1-96F8-0F3A23297376_=";
 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?bNqdaaEsAZPsYyLTfU2AmmVv7skW+WAU6BMz9LKCxrQtHXT6QfAIlXOZrS1S?=
 =?us-ascii?Q?5LjNDl58G3IwgFmZOJR9uAEP3yWv9lnGpCfDuAASsjah0ZpRnm22Z0kbf8U+?=
 =?us-ascii?Q?GCqT7j7DyaENdmjM0mVaco84lc4S33gpDDaHBlDJ8Gi2MRRc+lBHxllcU+Je?=
 =?us-ascii?Q?RW4iDobn4CrG3PVpz0nzKOpQVnt+JFETZo2aQ+AGpMUmVuHqTg77Zml2NxAw?=
 =?us-ascii?Q?vhf6QnSiGX4bwZqgrkKdrsuVSVbkJ4knC3C1/b/a7fERIVo4Zxtl0wfN5nug?=
 =?us-ascii?Q?cI0e7bEyKeQsqvlXtxfXCAaQ1ChshYjSxqa+yrWBa54EuWIO7eJifmj21+GE?=
 =?us-ascii?Q?WFwoTYeH1rjP3cZR5XePRJNK5ALBKk67+cfcLcodB9x9URJOLBshbKl9Xgjp?=
 =?us-ascii?Q?iITGDBDDmDei6xnRDvGoR4isPnAg0pcenIcpq/qLBx/tn+hZVB58l7tvOLYO?=
 =?us-ascii?Q?EuHbQOn3Ru2f8yF2mISJMN5d9e0Ea6wRXvTW1qmMM3hVNAnLKkKmMRRj7gkr?=
 =?us-ascii?Q?CCIlzxBRNnk6VOsw2PcO3ZFg0sw+m5Lg/1xefJHsvm+/dBfMYgkE6ysWQcJO?=
 =?us-ascii?Q?frskiw5c1fFcZEnYQSVkJyBF2ZEy8gSEWrBAyNZlDN+2qbFDCqbzPD3nnUUE?=
 =?us-ascii?Q?N2OJdor0CXcoT0bnl3ZXOBtORQ+c4H2RzeOreCrWGHTFfQD/trTaN2F/vUCd?=
 =?us-ascii?Q?lWpqZpRl8Hnf3OjZTEZIXbMLGvPT7TzPa0atR4+Oo+CWidhNKdlE8ufq3IPs?=
 =?us-ascii?Q?Ft1ILflN/JcX8YpBCcNW7iXrLUUcvQlOLnjJBYOHWLsIspRK6/gAq0krvhe+?=
 =?us-ascii?Q?4awrsoXAu9OO52gt/iGLQuxIvlQe7q8/e8xZj36yUWnlUzNEzgiy5Vjyv7bB?=
 =?us-ascii?Q?yut67j7gebrw108U3iGxH3X6U3LZlF1erPqG+kQWOEeHBpnYixkLoVdPBlJh?=
 =?us-ascii?Q?xynvVrd/uHiulhglbbM+k1ai2Nn3LtxAfE9LXC5NHIfKev8YRdP0L8Y8uBZI?=
 =?us-ascii?Q?1v6BsHZ7TBv746wb9uizz9zVNe810GULjy+ehLpoS2LR3IfEztKpnNu/bR3m?=
 =?us-ascii?Q?CXYBF+z954rMwg6lEzW59G1hCvBUoGYk680+FTArtf2PtzfkqHPUBWCpPJ96?=
 =?us-ascii?Q?ilzB3U2tkgl3+s6Rzuot44847zXfyC1jXzq0EvHNc1AVOTTClDBCwS/9Honn?=
 =?us-ascii?Q?wDr3lnl+6+4ITYz8J38Ig0wi5DLxupt6KEOagRJWJ6R4CI4w3433r4JBuhD2?=
 =?us-ascii?Q?KAY1Smoj3kzrRhB85LXZO/ZsAdVX4rc8LQGWkkJ3QY5TIuBZgZHeXkptRkBm?=
 =?us-ascii?Q?jAagQH201kjCzUmM4SyoYCihDVHYSZvaVZn4pFDQiZwVnwNy/WWC7trf5TO4?=
 =?us-ascii?Q?6jUfHZAe9hmoGu5YxOpnGcqz28BsqGf6WZQSMp0j3t5euUm9FELXfwgILYIN?=
 =?us-ascii?Q?4AjUHdLvY/Gphab3mcvo/KoVuPUwZXMHG01fBEqii4kvYkh7E8zGPLufTt6D?=
 =?us-ascii?Q?8LJx0pvpORQePTh1e7XNLzUG/JKE3hGc6M40KxzBHp25x7rnzUvavqEe5KB8?=
 =?us-ascii?Q?QLUly56VynVXPfMsFgyf8Do4RkQ3Thyvp4otPK5l?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fc32d2c4-9c45-445f-ef87-08dc6aaa251c
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2024 13:16:58.0598
 (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: FOAuerlTg1nMdRtdj2rSuUVPWtWYhkqEm9DuuuN3ZdcZFd1vF1523/D9CSETmIvZ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR12MB8957

--=_MailMate_38CF2DC2-489D-4DC1-96F8-0F3A23297376_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 2 May 2024, at 3:33, Ryan Roberts wrote:

> On 02/05/2024 02:27, Zi Yan wrote:
>> On 1 May 2024, at 10:33, 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
>>> 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.
>>>
>>> x86 does not suffer the above problem, but instead pmd_mkinvalid() wi=
ll
>>> corrupt the offset field of the swap entry within the swap pte. See l=
ink
>>> below for discussion of that problem.
>>>
>>> Fix all of this by only calling pmdp_invalidate() for a present pmd. =
And
>>> for good measure let's add a warning to all implementations of
>>> pmdp_invalidate[_ad](). 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.
>>>
>>> Cc: stable@vger.kernel.org
>>> Link: https://lore.kernel.org/all/0dd7827a-6334-439a-8fd0-43c98e6af22=
b@arm.com/
>>> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common pa=
th")
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>
>>> Right v3; this goes back to the original approach in v1 to fix core-m=
m rather
>>> than push the fix into arm64, since we discovered that x86 can't hand=
le
>>> pmd_mkinvalid() being called for non-present pmds either.
>>>
>>> I'm pulling in more arch maintainers because this version adds some w=
arnings in
>>> arch code to help spot incorrect usage.
>>>
>>> Although Catalin had already accepted v2 (fixing arm64) [2] into for-=
next/fixes,
>>> he's agreed to either remove or revert it.
>>>
>>>
>>> Changes since v1 [1]
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>
>>>   - Improve pmdp_mkinvalid() docs to make it clear it can only be cal=
led for
>>>     present pmd (per JohnH, Zi Yan)
>>>   - Added warnings to arch overrides of pmdp_invalidate[_ad]() (per Z=
i Yan)
>>>   - Moved comment next to new location of pmpd_invalidate() (per Zi Y=
an)
>>>
>>>
>>> [1] https://lore.kernel.org/linux-mm/20240425170704.3379492-1-ryan.ro=
berts@arm.com/
>>> [2] https://lore.kernel.org/all/20240430133138.732088-1-ryan.roberts@=
arm.com/
>>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>>  Documentation/mm/arch_pgtable_helpers.rst |  6 ++-
>>>  arch/powerpc/mm/book3s64/pgtable.c        |  1 +
>>>  arch/s390/include/asm/pgtable.h           |  4 +-
>>>  arch/sparc/mm/tlb.c                       |  1 +
>>>  arch/x86/mm/pgtable.c                     |  2 +
>>>  mm/huge_memory.c                          | 49 ++++++++++++---------=
--
>>>  mm/pgtable-generic.c                      |  2 +
>>>  7 files changed, 39 insertions(+), 26 deletions(-)
>>
>> The changes in Documentation/mm and mm/* look good to me. Thanks.
>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>
> Thanks!
>
>>
>> I wonder if making Documentation/mm and mm/* changes in a separate pat=
ch
>> would be better, since you will not need acks from arch maintainers an=
d
>> get the patch in quicker.
>
> Yeah maybe - I considered that, but then decided I'm literally just add=
ing a
> debug warning to the arch code so it shouldn't be too controversial. Pe=
rhaps
> wait a few days for acks and if nothing turns up then I'll re-post with=
 it split?
Sure, sounds good to me. Thanks.

--
Best Regards,
Yan, Zi

--=_MailMate_38CF2DC2-489D-4DC1-96F8-0F3A23297376_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYzkkcPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUvMcQAKJrLAR7F//CCGv8JJrUq4GMdZKP2ksQMqzc
/sjrwG53foQQubulasS04PHXGLoZWJt671SyLKUw7bvtmRalOxM6PuA84KY0Y8IS
FPwATA598nak9S/+WaDSybomtPIVZDTWgNYigTi8Wq9NR3x/Z1nBLJ2WPXNm/LiN
Q4ZQh0DjBrGKTrq7KDTGRdXgkFkAoc7XLBBQSb9VtKU1+2DisZsJw95SO5osTM3S
V9uVtlpYJdTbMuX96ZrQZNNVd7RvwWfOljGf7iTFXdQUMlZg9aX2B8/BLWxIji96
98hGBA1Tk9BnBHwYkJd4Et/O1D0gsfLuT+f1eoQvwuPEfGaEgXpT4LeqP3twmm+3
/jXjP+idt4cfvrKoM87D09iMTFTZrH7H2HCRcjhWbImyoa68F3OiA7vyI2wox0MX
Gpm58KmVUqgbFe5pe/HvN9aldqjhzGGOjnxrBzeDJZ+OmtMG8KSaFC8s5vDM3a6Z
QGkacX0dxt5Yg5VWvb5Ab+htkI1WXEVo+GavjZYFjHowi6p++Ka1jmhSWk4rxYBm
jCZ+3s1WJgKhBET3UrZE3aFXTqkpNodTjPcZh02dyDdOi0PoTRb1shfau+eLKcdT
3bOqy7U68zJHYY+PuDgq0fS5ZwEEkvXpo/0/Cn5eMZ2v1r1eatGdFnTiyzyaOC9R
+ae2bhll
=egvu
-----END PGP SIGNATURE-----

--=_MailMate_38CF2DC2-489D-4DC1-96F8-0F3A23297376_=--