Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <cover.f15b25597fc3afd45b144df863eeca3b2c13f9f4.1664171943.git-series.apopple@nvidia.com>
 <af2ea89799b08e0a5e592df0da0dcb9a5bf8533b.1664171943.git-series.apopple@nvidia.com>
 <87fsgbf3gh.fsf@mpe.ellerman.id.au>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Michael Ellerman <mpe@ellerman.id.au>
Cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        Nicholas Piggin <npiggin@gmail.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Christian =?utf-8?Q?K=C3=B6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Ben Skeggs <bskeggs@redhat.com>,
        Karol Herbst <kherbst@redhat.com>,
        Lyude Paul <lyude@redhat.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Alex Sierra <alex.sierra@amd.com>,
        John Hubbard <jhubbard@nvidia.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, Jason Gunthorpe <jgg@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCH 1/7] mm/memory.c: Fix race when faulting a device
 private page
Date:   Thu, 29 Sep 2022 11:40:32 +1000
In-reply-to: <87fsgbf3gh.fsf@mpe.ellerman.id.au>
Message-ID: <875yh7osye.fsf@nvdebian.thelocal>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ZLsjQMvroX3692nr4vs6euCOhsYvI0+5+C3uhflRIknhPheIMc4/dGYFMhmx?=
 =?us-ascii?Q?zPB4CMAP4fTQGkG93jcvLgSpBKDMed/Fd/R7z72BxCCXxyFHjy7HRkUn/m6i?=
 =?us-ascii?Q?2nnpioFqIa/mRSFe+fvyErEiOGkNSfQ99YTg5GrdsANSvQB2KttnbfyjcCP9?=
 =?us-ascii?Q?g6+njkk+g/vlSdXmRrHhJ3PJ3E/GuwCKostRJHG/PxvCSBmkzRMJbl/YV1DP?=
 =?us-ascii?Q?IPmVw48j+K2cSx5U2YVrosquvQTQ+O+cjSjndelSr11aG66tnELfryIqm6tc?=
 =?us-ascii?Q?kyY3+7QDdmCRRKoSiGheRwZqwezHfYfxxngwpm8mbtJBiXHJuzEYiLJcgNnx?=
 =?us-ascii?Q?Qsr3BnTZr++/iXJ5XY7NAoXnofMXuDiGnjSfLoIlIsXP4/3PBh40mfspSZDu?=
 =?us-ascii?Q?4Xf7AiEfKSa/regLuHeMTjHF5IZeyyJC9ogMUqUPvD/8cU/1ihfghF0q8Y8/?=
 =?us-ascii?Q?A9s8F/mjyKJV2c3HNWvPwDxh43VfPWnc6WXDeF4eAN19XTx33sWdfREf/P0H?=
 =?us-ascii?Q?RLyzWKBkACfHdCziqP1BT4dbyPt0sl5Rag6/PwsmOFJZcX4Oqp1Kc+v9EW6k?=
 =?us-ascii?Q?G81b8HQTiV/qGmqu88Yfa0IeXgnc344FJNqmbyy0zGEMpMcD0B8fBbEWXNwS?=
 =?us-ascii?Q?BSnptxkOsIxDRwc6H8dfIe/d+ggMnImDIK/cGmQnNrdnGC8F61J+5HAcGjXb?=
 =?us-ascii?Q?mRh19Am3jCURgBhKOdFau4ILtsdD2iU49kZEU+wb+gMot5OmX9q8OaXH7/I6?=
 =?us-ascii?Q?lcEt9Ig+SKn3+RV3tEasslo5nHaWizWQLLAOzyXRwkVoams3P956fIMm4+WS?=
 =?us-ascii?Q?DJwsxDg1ZV13HLgzXaN9y90hL3x2H4JT4oSujQqOYobtQWMnm85Tee+CEU6W?=
 =?us-ascii?Q?yheC6m0HyVIRVgLH8w/tVgf/KGoa5tIXWjeAKRxYrs+R0alHYj7DN+K8g4V0?=
 =?us-ascii?Q?VCJnxIIG30kz47bwzfMeT39XCf8FpHugyCryT0tr6EjQs6so1b95Jq48pDni?=
 =?us-ascii?Q?pgPA2tPQVq+GdHQPMxptJk3unYK0HNcaoR1uAAXTYcrGrYD/1TIfNABLRpgA?=
 =?us-ascii?Q?XAegzWld47hZXw6ZpVa0SU2KGnuiR8hh3sdsH45fXKwSJWpOhrmbZ37XyRoI?=
 =?us-ascii?Q?G+CwGJMCwT5uHJSF8Y6TScRHp9yMUHCH5pWYfnxnSPsXtZvzmJB0lahyV/UL?=
 =?us-ascii?Q?Ed1k4/SB3ASlofEM0Pf7V4p8nQj+YBuM2x04roS52JZ92E5OBAkDsC53jv6K?=
 =?us-ascii?Q?XoiIectsoEF1uoTB82smxuGyV2dcZAAdLG0bqT2kX8Efk8TMa9EFPG8lw+Q0?=
 =?us-ascii?Q?KszN2loighEnV78rUvvzEHgPSmV/lDzUhszYL5lU4krhiLZPNFUB0yqohBi5?=
 =?us-ascii?Q?LvDgw1MkdeVwmb48ULjOeaXbsuA/9IOT2f3TGwt0AQVzrnm45O6g0e1wMP7m?=
 =?us-ascii?Q?Upe/64rt+IDgoQmQO0AMvLDrNbp8OqSt8vuMlx5NIN0OtunpxCgvUwXRvLmk?=
 =?us-ascii?Q?pzF5J0/7BHECi9IlmoEqHU31aXeWDSYL/EZ4znihtkAqbA+TmzJErG4aPsFQ?=
 =?us-ascii?Q?0zWscIAhNSDUu1mFNRWueWO9cILrczkjJgOnp7SU?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e82023d0-6226-43b8-c76d-08daa1bc1e11
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 01:44:16.0657
 (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: trBmO7349RJmQc3OQ8ygytOuPx62JiJUbdRBR/MSSEueeUm1EeDiVHeiStTuw5C/egOBT5NmiFrFi4wNSi7DZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5106
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Michael Ellerman <mpe@ellerman.id.au> writes:

> Alistair Popple <apopple@nvidia.com> writes:
>> When the CPU tries to access a device private page the migrate_to_ram()
>> callback associated with the pgmap for the page is called. However no
>> reference is taken on the faulting page. Therefore a concurrent
>> migration of the device private page can free the page and possibly the
>> underlying pgmap. This results in a race which can crash the kernel due
>> to the migrate_to_ram() function pointer becoming invalid. It also means
>> drivers can't reliably read the zone_device_data field because the page
>> may have been freed with memunmap_pages().
>>
>> Close the race by getting a reference on the page while holding the ptl
>> to ensure it has not been freed. Unfortunately the elevated reference
>> count will cause the migration required to handle the fault to fail. To
>> avoid this failure pass the faulting page into the migrate_vma functions
>> so that if an elevated reference count is found it can be checked to see
>> if it's expected or not.
>>
>> Signed-off-by: Alistair Popple <apopple@nvidia.com>
>> ---
>>  arch/powerpc/kvm/book3s_hv_uvmem.c       | 15 ++++++-----
>>  drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++++++------
>>  drivers/gpu/drm/amd/amdkfd/kfd_migrate.h |  2 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 11 +++++---
>>  include/linux/migrate.h                  |  8 ++++++-
>>  lib/test_hmm.c                           |  7 ++---
>>  mm/memory.c                              | 16 +++++++++++-
>>  mm/migrate.c                             | 34 ++++++++++++++-----------
>>  mm/migrate_device.c                      | 18 +++++++++----
>>  9 files changed, 87 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> index 5980063..d4eacf4 100644
>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> @@ -508,10 +508,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
>>  static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>>  		unsigned long start,
>>  		unsigned long end, unsigned long page_shift,
>> -		struct kvm *kvm, unsigned long gpa)
>> +		struct kvm *kvm, unsigned long gpa, struct page *fault_page)
>>  {
>>  	unsigned long src_pfn, dst_pfn = 0;
>> -	struct migrate_vma mig;
>> +	struct migrate_vma mig = { 0 };
>>  	struct page *dpage, *spage;
>>  	struct kvmppc_uvmem_page_pvt *pvt;
>>  	unsigned long pfn;
>> @@ -525,6 +525,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>>  	mig.dst = &dst_pfn;
>>  	mig.pgmap_owner = &kvmppc_uvmem_pgmap;
>>  	mig.flags = MIGRATE_VMA_SELECT_DEVICE_PRIVATE;
>> +	mig.fault_page = fault_page;
>>
>>  	/* The requested page is already paged-out, nothing to do */
>>  	if (!kvmppc_gfn_is_uvmem_pfn(gpa >> page_shift, kvm, NULL))
>> @@ -580,12 +581,14 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma,
>>  static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
>>  				      unsigned long start, unsigned long end,
>>  				      unsigned long page_shift,
>> -				      struct kvm *kvm, unsigned long gpa)
>> +				      struct kvm *kvm, unsigned long gpa,
>> +				      struct page *fault_page)
>>  {
>>  	int ret;
>>
>>  	mutex_lock(&kvm->arch.uvmem_lock);
>> -	ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa);
>> +	ret = __kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa,
>> +				fault_page);
>>  	mutex_unlock(&kvm->arch.uvmem_lock);
>>
>>  	return ret;
>> @@ -736,7 +739,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma,
>>  		bool pagein)
>>  {
>>  	unsigned long src_pfn, dst_pfn = 0;
>> -	struct migrate_vma mig;
>> +	struct migrate_vma mig = { 0 };
>>  	struct page *spage;
>>  	unsigned long pfn;
>>  	struct page *dpage;
>> @@ -994,7 +997,7 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
>>
>>  	if (kvmppc_svm_page_out(vmf->vma, vmf->address,
>>  				vmf->address + PAGE_SIZE, PAGE_SHIFT,
>> -				pvt->kvm, pvt->gpa))
>> +				pvt->kvm, pvt->gpa, vmf->page))
>>  		return VM_FAULT_SIGBUS;
>>  	else
>>  		return 0;
>
> I don't have a UV test system, but as-is it doesn't even compile :)

Ugh, thanks. I did get as far as installing a PPC cross-compiler and
building a kernel. Apparently I did not get as far as enabling
CONFIG_PPC_UV :)

> kvmppc_svm_page_out() is called via some paths other than the
> migrate_to_ram callback.
>
> I think it's correct to just pass fault_page = NULL when it's not called
> from the migrate_to_ram callback?
>
> Incremental diff below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index d4eacf410956..965c9e9e500b 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -637,7 +637,7 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot,
>  			pvt->remove_gfn = true;
>
>  			if (__kvmppc_svm_page_out(vma, addr, addr + PAGE_SIZE,
> -						  PAGE_SHIFT, kvm, pvt->gpa))
> +						  PAGE_SHIFT, kvm, pvt->gpa, NULL))
>  				pr_err("Can't page out gpa:0x%lx addr:0x%lx\n",
>  				       pvt->gpa, addr);
>  		} else {
> @@ -1068,7 +1068,7 @@ kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gpa,
>  	if (!vma || vma->vm_start > start || vma->vm_end < end)
>  		goto out;
>
> -	if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa))
> +	if (!kvmppc_svm_page_out(vma, start, end, page_shift, kvm, gpa, NULL))
>  		ret = H_SUCCESS;
>  out:
>  	mmap_read_unlock(kvm->mm);