Received-SPF: pass (google.com: domain of linux-kernel+bounces-160889-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <1734de6e-1bcd-492e-b07b-9ad712967e1d@nvidia.com>
Date: Fri, 26 Apr 2024 21:25:41 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and
 GUP-fast
To: Zi Yan <ziy@nvidia.com>, Ryan Roberts <ryan.roberts@arm.com>
CC: Anshuman Khandual <anshuman.khandual@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>
References: <20240425170704.3379492-1-ryan.roberts@arm.com>
 <39d0236c-4297-4001-bc7e-f516bc674f7b@arm.com>
 <87df74b7-2352-4ad6-b9f9-baed9291cbed@arm.com>
 <80148272-208A-4306-A8EC-98C79BBBDCA0@nvidia.com>
 <D0E6B5DC-6206-408C-81D1-C9B0A85E1D0E@nvidia.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <D0E6B5DC-6206-408C-81D1-C9B0A85E1D0E@nvidia.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2024 04:25:53.7443
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 208b195e-bd5b-40a9-4e00-08dc6672209e
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	CH2PEPF00000142.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7484

On 4/26/24 7:53 AM, Zi Yan wrote:

Hi Zi (and Ryan)!

>>>>> 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.
>>>>
>>>> Could you please explain how bad things might happen ?
>>>
>>> See 2 places where pmdp_get_lockless() is called in gup.c, without the PTL.
>>> These could both return the swap pte for which pmd_mkinvalid() has been called.
>>> In both cases, this would lead to the pmd_present() check eroneously returning
>>> true, eventually causing incorrect interpretation of the pte fields. e.g.:
>>>
>>> gup_pmd_range()
>>>    pmd_t pmd = pmdp_get_lockless(pmdp);
>>>    gup_huge_pmd(pmd, ...)
>>>      page = nth_page(pmd_page(orig), (addr & ~PMD_MASK) >> PAGE_SHIFT);
>>>
>>> page is guff.
>>>
>>> Let me know what you think!
> 
> Add JohnH to check GUP code.

Ryan is correct about this behavior.

By the way, remember that gup is not the only lockless page table
walker: there is also the CPU hardware itself, which inconveniently
refuses to bother with taking page table locks. :)

So if we have code that can make a non-present PTE appear to be present
to any of these page walkers, whether software or hardware, it's a
definitely Not Good and will lead directly to bugs.

Since I had to study this patch and discussion a bit in order to
respond, I'll go ahead and also reply to the original patch with review
comments.


thanks,
-- 
John Hubbard
NVIDIA