Received-SPF: pass (google.com: domain of linux-kernel+bounces-160890-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.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <17956e0f-1101-42d7-9cba-87e196312484@nvidia.com>
Date: Fri, 26 Apr 2024 21:41:30 -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: Ryan Roberts <ryan.roberts@arm.com>, Andrew Morton
	<akpm@linux-foundation.org>, Zi Yan <zi.yan@cs.rutgers.edu>, Aneesh Kumar K.V
	<aneesh.kumar@kernel.org>
CC: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>
References: <20240425170704.3379492-1-ryan.roberts@arm.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20240425170704.3379492-1-ryan.roberts@arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2024 04:41:39.2500
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: f0c4962c-1fad-4b91-e0e9-08dc66745427
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.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	SA2PEPF000015C9.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB6897

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 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 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).

> good measure let's add a warning to the generic implementation of
> pmdp_invalidate(). 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.
> 
> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
> 
> Applies on top of v6.9-rc5. Passes all the mm selftests on arm64.
> 
> Thanks,
> Ryan
> 
> 
>   mm/huge_memory.c     | 5 +++--
>   mm/pgtable-generic.c | 2 ++
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 89f58c7603b2..80939ad00718 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2513,12 +2513,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   	 * for this pmd), then we flush the SMP TLB and finally we write the
>   	 * non-huge version of the pmd entry with pmd_populate.
>   	 */
> -	old_pmd = pmdp_invalidate(vma, haddr, pmd);
> 
> -	pmd_migration = is_pmd_migration_entry(old_pmd);
> +	pmd_migration = is_pmd_migration_entry(*pmd);
>   	if (unlikely(pmd_migration)) {
>   		swp_entry_t entry;
> 
> +		old_pmd = *pmd;
>   		entry = pmd_to_swp_entry(old_pmd);
>   		page = pfn_swap_entry_to_page(entry);
>   		write = is_writable_migration_entry(entry);
> @@ -2529,6 +2529,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   		soft_dirty = pmd_swp_soft_dirty(old_pmd);
>   		uffd_wp = pmd_swp_uffd_wp(old_pmd);
>   	} else {
> +		old_pmd = pmdp_invalidate(vma, haddr, pmd);

This looks good, except that now I am deeply confused about the pre-existing
logic. I thought that migration entries were a subset of swap entries,
but this code seems to be treating is_pmd_migration_entry() as a
synonym for "is a swap entry". Can you shed any light on this for me?


>   		page = pmd_page(old_pmd);
>   		folio = page_folio(page);
>   		if (pmd_dirty(old_pmd)) {
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index 4fcd959dcc4d..74e34ea90656 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -198,6 +198,7 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
>   pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>   		     pmd_t *pmdp)
>   {
> +	VM_WARN_ON(!pmd_present(*pmdp));
>   	pmd_t old = pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmdp));
>   	flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
>   	return old;
> @@ -208,6 +209,7 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>   pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma, unsigned long address,
>   			 pmd_t *pmdp)
>   {
> +	VM_WARN_ON(!pmd_present(*pmdp));

Should these be VM_WARN_ON_ONCE(), instead?

Also, this seems like a good place to put a little comment in, to mark the
new design constraint. Something like "Only present entries are allowed
to be invalidated", perhaps.


>   	return pmdp_invalidate(vma, address, pmdp);
>   }
>   #endif
> --
> 2.25.1
> 
> 

thanks,
-- 
John Hubbard
NVIDIA