Received-SPF: pass (google.com: domain of linux-kernel+bounces-165897-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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: Wed, 01 May 2024 21:27:26 -0400
Message-ID: <4992C22D-D7B7-4B6F-8A50-E084163BEF42@nvidia.com>
In-Reply-To: <20240501143310.1381675-1-ryan.roberts@arm.com>
References: <20240501143310.1381675-1-ryan.roberts@arm.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_8785460F-D7C1-49E2-A915-6322DAE5CB54_=";
 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?gsQdxMwJo2aPEavqOv77d0uSBSxl0rzkPFoIUNHVGcRfEz/qg1e8pJITUp4p?=
 =?us-ascii?Q?afpY7i9qt3faJ0pQkE8p616cJ++BmP9Ij+542V9OXUgS7M7ikvC4FpZ3PSUA?=
 =?us-ascii?Q?iw3t9KALb17FwlbaW7cmfXn2hgIBqbqSIB9OM1I0urYeq41d8T69BwHUTZhr?=
 =?us-ascii?Q?Q3omUcUMsrvXlj7mtS6SOxo/dImYZ+aFlR9J9DpHwOvvB6fktxmivGwmxQ7r?=
 =?us-ascii?Q?HfQI8foq43UrruQk1p0BEapawb0JwL9oeVaidfs9MaGzOBApGaRuchIeImc8?=
 =?us-ascii?Q?Gypl2gLz/8NafSkIUtcmk8JQJ2mklOEQCZWVDw93Srpd/RKkiSZor3a7iuMb?=
 =?us-ascii?Q?uYcgZfTAi3yy0FzH8RwUVri7GDdwmUQ7LFjPJNyRAXh0H/nU7MFNjtP+HRto?=
 =?us-ascii?Q?xx0u6VLjgQb8Acj26txax30ZFanF3IvJLjECllq+9nRJcWjYNyNboQGr9y/g?=
 =?us-ascii?Q?nWbFcg9YupfWTwgEt4AvPRW7jJm4GCXx2Kv17BUQBmkxgDZTN+2DDM0v3cO4?=
 =?us-ascii?Q?uvikw4E+yTwYFOoZb749MoGeCTq5D/DIoMRSgPoOgu1kIuHSIMXr03Jl2/0Q?=
 =?us-ascii?Q?bRJ40TGPa+MuyUNYJY3mhL9xfBOUqGukeOghCQabXNYfKVMPqcMjl+nVZNsL?=
 =?us-ascii?Q?zYzRLR74slR+9MjumF26ZbziW/ZAXMH8z8rF0C4X+hUsZ3InkOuq43lHDQHK?=
 =?us-ascii?Q?vJwxRp05TQl9GOrUL2wLnDYABZL54fc5VjdGGxqlPWK3lQn5qw0KKAgRsQ9k?=
 =?us-ascii?Q?j2p8cixp3oxk/eP8OME1A7EQcdlXClIkdLyXXqYKQh6V4v6xlga+mLrNxKMn?=
 =?us-ascii?Q?AygmMgnN9+9cfxVWFaUDvwLUiUO2UV06fgdmmchbFs+vwpIfMSZg2ooXiVWs?=
 =?us-ascii?Q?sdt0nbaMZ+CgGwjYs0fpRC9xEI8fHNBHrJgzxL9RHlC1sE2PnBVqqP/jHcUD?=
 =?us-ascii?Q?ahXv50Bixf+p8+cNIYQRGAkj7oMaoSQO2GADS01nflsZR2YAtPYt2IKbQZv7?=
 =?us-ascii?Q?J5OinNL1bZqJ28+i/Q/A9DsVIuTRCI+KcJ2KZ6Nzr9GC16bdDp3cTDzaUw2u?=
 =?us-ascii?Q?8Cs0z9jgTmNE512idvu9lCsWXXVZ99gtK6+DxLcTLN6uttSvtV69RN82N2lw?=
 =?us-ascii?Q?i9NUjB37dUZKQO4oWbu0/wPPA6ouVCkrdTf8rCA1mwvLF738THCdhAdf/W4v?=
 =?us-ascii?Q?orYwd2bbRNRROHqHNXYhsUhH2CJdFtjrDM+oEHQp551nGbRodyZaHgZSR+iQ?=
 =?us-ascii?Q?PAhMWkTpO7HdtfoqTaGaTiEAQDoXinJqJzeh1xH4AB4DWQu2IiejeyfZmnXh?=
 =?us-ascii?Q?ytaS4KHE/K0vur30xjdPJ+BuUIRUl3vzyx6UAKR5WHplB+MyabvgBIz/H4fQ?=
 =?us-ascii?Q?DjIIvd62KZ83lK6XtiCTp8Y/LN3slH372npQJyXZlVju7zvza1eBsGKyDRWa?=
 =?us-ascii?Q?QqrlNEQWP2SXMiaUFCH6TDDCPNsRWbOWavqyYctFih7Wv+0ZfeLcYpyVl+a0?=
 =?us-ascii?Q?+ylsmM788+xRv0JvtXAPPBFGrb0U2cmIIkf6WR8kgQ8a9LK8duyqaeVTLB6t?=
 =?us-ascii?Q?VT8OmIaQ/AJZyRBS8ySJLgMcBbifo9zTmJmovezJ?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 43656432-17d9-46d7-c3c2-08dc6a47081c
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2024 01:27:29.2441
 (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: GDXbk5hmjjSzPXlnbGSSAzpyLE/fck8kwj/2QpIJda7LZJly/OS+9E+507lJCfv9
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6432

--=_MailMate_8785460F-D7C1-49E2-A915-6322DAE5CB54_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

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 not=

> based on the returned old pmd. This is a problem for the migration entr=
y
> 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 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=
=2E
>
> x86 does not suffer the above problem, but instead pmd_mkinvalid() will=

> corrupt the offset field of the swap entry within the swap pte. See lin=
k
> below for discussion of that problem.
>
> Fix all of this by only calling pmdp_invalidate() for a present pmd. An=
d
> 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-43c98e6af22b@=
arm.com/
> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path=
")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>
> Right v3; this goes back to the original approach in v1 to fix core-mm =
rather
> than push the fix into arm64, since we discovered that x86 can't handle=

> pmd_mkinvalid() being called for non-present pmds either.
>
> I'm pulling in more arch maintainers because this version adds some war=
nings in
> arch code to help spot incorrect usage.
>
> Although Catalin had already accepted v2 (fixing arm64) [2] into for-ne=
xt/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 calle=
d for
>     present pmd (per JohnH, Zi Yan)
>   - Added warnings to arch overrides of pmdp_invalidate[_ad]() (per Zi =
Yan)
>   - Moved comment next to new location of pmpd_invalidate() (per Zi Yan=
)
>
>
> [1] https://lore.kernel.org/linux-mm/20240425170704.3379492-1-ryan.robe=
rts@arm.com/
> [2] https://lore.kernel.org/all/20240430133138.732088-1-ryan.roberts@ar=
m.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>

I wonder if making Documentation/mm and mm/* changes in a separate patch
would be better, since you will not need acks from arch maintainers and
get the patch in quicker.


--
Best Regards,
Yan, Zi

--=_MailMate_8785460F-D7C1-49E2-A915-6322DAE5CB54_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmYy6/8PHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUj24QAJ7zbilMbFHAiSNHI58wYRAes6x3XF1qpHv/
7XFJdwPikMP2pjg+YZ8XDWU/1sLliGusA5fcOqIrVHwrgvQO82KB6bRN8+3hEiNZ
VqQ6u+jtHzWlskIQCW0v0JFJ2k2b19uaLzKV/IxA03WfwByrJzvDnELu1tpGg75R
wGmdNvBJda0urGIaSr0prpMmnrvozSaXDvnq/dscNLqHdoEMtvRXFJ3z9lUPXbqC
2GnxruaWHSl/Jq0jRU+Mf5UGmMnoXTQ9Fa7Xbv2qGMmN/bIVqAwIsNMF8r8S95s4
bOeUa97BOqTRyVUUFX/3lGhnm4l7u64tIpX/7TfSJtwspPA6XGneERWGFIyaBwP1
7ItaFpUxJUaUiNYrZjoMo2GKqbEr4sUDaShGMW2Heqdhzr28kWDJfZ9LoQ4yBchH
bEGCvF2yb2MsWkGICmfZgOJ35prUdxxz2VwcSBWJj3nTlvs+CnkSmYwu0Iifowah
cUTEpqMK39ItYpbfgZfTD2A3OO8XbradD/q3NFbj2/d0g+ybJozIxZ3nqytaQBHr
7Rj6C7LeatfMd+59vmV0L8gTHn4ilQstLMENyFizE1Ysx8iDSM9Cnd8NkjacYZcD
P+XTK7wYbj8pjvaAjKyUdDh6TknQu77QCMS6vGAy1U7fEnBt6uiTvMdjUQR3u4jx
5re3JIIp
=ZG64
-----END PGP SIGNATURE-----

--=_MailMate_8785460F-D7C1-49E2-A915-6322DAE5CB54_=--