| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7ca6ec0c-7e5e-3b24-8f8d-650df357130c@amd.com>
Date: Mon, 26 Sep 2022 19:07:23 -0400
From: Felix Kuehling <felix.kuehling@....com>
To: Lyude Paul <lyude@...hat.com>,
Alistair Popple <apopple@...dia.com>, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
"Pan, Xinhui" <Xinhui.Pan@....com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Ben Skeggs <bskeggs@...hat.com>,
Karol Herbst <kherbst@...hat.com>,
Ralph Campbell <rcampbell@...dia.com>,
"Matthew Wilcox (Oracle)" <willy@...radead.org>,
Alex Sierra <alex.sierra@....com>,
John Hubbard <jhubbard@...dia.com>,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
amd-gfx@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, Jason Gunthorpe <jgg@...dia.com>,
Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH 6/7] nouveau/dmem: Evict device private memory during
release
On 2022-09-26 17:35, Lyude Paul wrote:
> On Mon, 2022-09-26 at 16:03 +1000, Alistair Popple wrote:
>> When the module is unloaded or a GPU is unbound from the module it is
>> possible for device private pages to be left mapped in currently running
>> processes. This leads to a kernel crash when the pages are either freed
>> or accessed from the CPU because the GPU and associated data structures
>> and callbacks have all been freed.
>>
>> Fix this by migrating any mappings back to normal CPU memory prior to
>> freeing the GPU memory chunks and associated device private pages.
>>
>> Signed-off-by: Alistair Popple <apopple@...dia.com>
>>
>> ---
>>
>> I assume the AMD driver might have a similar issue. However I can't see
>> where device private (or coherent) pages actually get unmapped/freed
>> during teardown as I couldn't find any relevant calls to
>> devm_memunmap(), memunmap(), devm_release_mem_region() or
>> release_mem_region(). So it appears that ZONE_DEVICE pages are not being
>> properly freed during module unload, unless I'm missing something?
> I've got no idea, will poke Ben to see if they know the answer to this
I guess we're relying on devm to release the region. Isn't the whole
point of using devm_request_free_mem_region that we don't have to
remember to explicitly release it when the device gets destroyed? I
believe we had an explicit free call at some point by mistake, and that
caused a double-free during module unload. See this commit for reference:
commit 22f4f4faf337d5fb2d2750aff13215726814273e
Author: Philip Yang <Philip.Yang@....com>
Date: Mon Sep 20 17:25:52 2021 -0400
drm/amdkfd: fix svm_migrate_fini warning
Device manager releases device-specific resources when a driver
disconnects from a device, devm_memunmap_pages and
devm_release_mem_region calls in svm_migrate_fini are redundant.
It causes below warning trace after patch "drm/amdgpu: Split
amdgpu_device_fini into early and late", so remove function
svm_migrate_fini.
BUG: https://gitlab.freedesktop.org/drm/amd/-/issues/1718
WARNING: CPU: 1 PID: 3646 at drivers/base/devres.c:795
devm_release_action+0x51/0x60
Call Trace:
? memunmap_pages+0x360/0x360
svm_migrate_fini+0x2d/0x60 [amdgpu]
kgd2kfd_device_exit+0x23/0xa0 [amdgpu]
amdgpu_amdkfd_device_fini_sw+0x1d/0x30 [amdgpu]
amdgpu_device_fini_sw+0x45/0x290 [amdgpu]
amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
drm_dev_release+0x20/0x40 [drm]
release_nodes+0x196/0x1e0
device_release_driver_internal+0x104/0x1d0
driver_detach+0x47/0x90
bus_remove_driver+0x7a/0xd0
pci_unregister_driver+0x3d/0x90
amdgpu_exit+0x11/0x20 [amdgpu]
Signed-off-by: Philip Yang <Philip.Yang@....com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@....com>
Signed-off-by: Alex Deucher <alexander.deucher@....com>
Furthermore, I guess we are assuming that nobody is using the GPU when
the module is unloaded. As long as any processes have /dev/kfd open, you
won't be able to unload the module (except by force-unload). I suppose
with ZONE_DEVICE memory, we can have references to device memory pages
even when user mode has closed /dev/kfd. We do have a cleanup handler
that runs in an MMU-free-notifier. In theory that should run after all
the pages in the mm_struct have been freed. It releases all sorts of
other device resources and needs the driver to still be there. I'm not
sure if there is anything preventing a module unload before the
free-notifier runs. I'll look into that.
Regards,
Felix
>
>> ---
>> drivers/gpu/drm/nouveau/nouveau_dmem.c | 48 +++++++++++++++++++++++++++-
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> index 66ebbd4..3b247b8 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> @@ -369,6 +369,52 @@ nouveau_dmem_suspend(struct nouveau_drm *drm)
>> mutex_unlock(&drm->dmem->mutex);
>> }
>>
>> +/*
>> + * Evict all pages mapping a chunk.
>> + */
>> +void
>> +nouveau_dmem_evict_chunk(struct nouveau_dmem_chunk *chunk)
>> +{
>> + unsigned long i, npages = range_len(&chunk->pagemap.range) >> PAGE_SHIFT;
>> + unsigned long *src_pfns, *dst_pfns;
>> + dma_addr_t *dma_addrs;
>> + struct nouveau_fence *fence;
>> +
>> + src_pfns = kcalloc(npages, sizeof(*src_pfns), GFP_KERNEL);
>> + dst_pfns = kcalloc(npages, sizeof(*dst_pfns), GFP_KERNEL);
>> + dma_addrs = kcalloc(npages, sizeof(*dma_addrs), GFP_KERNEL);
>> +
>> + migrate_device_range(src_pfns, chunk->pagemap.range.start >> PAGE_SHIFT,
>> + npages);
>> +
>> + for (i = 0; i < npages; i++) {
>> + if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
>> + struct page *dpage;
>> +
>> + /*
>> + * _GFP_NOFAIL because the GPU is going away and there
>> + * is nothing sensible we can do if we can't copy the
>> + * data back.
>> + */
> You'll have to excuse me for a moment since this area of nouveau isn't one of
> my strongpoints, but are we sure about this? IIRC __GFP_NOFAIL means infinite
> retry, in the case of a GPU hotplug event I would assume we would rather just
> stop trying to migrate things to the GPU and just drop the data instead of
> hanging on infinite retries.
>
>> + dpage = alloc_page(GFP_HIGHUSER | __GFP_NOFAIL);
>> + dst_pfns[i] = migrate_pfn(page_to_pfn(dpage));
>> + nouveau_dmem_copy_one(chunk->drm,
>> + migrate_pfn_to_page(src_pfns[i]), dpage,
>> + &dma_addrs[i]);
>> + }
>> + }
>> +
>> + nouveau_fence_new(chunk->drm->dmem->migrate.chan, false, &fence);
>> + migrate_device_pages(src_pfns, dst_pfns, npages);
>> + nouveau_dmem_fence_done(&fence);
>> + migrate_device_finalize(src_pfns, dst_pfns, npages);
>> + kfree(src_pfns);
>> + kfree(dst_pfns);
>> + for (i = 0; i < npages; i++)
>> + dma_unmap_page(chunk->drm->dev->dev, dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL);
>> + kfree(dma_addrs);
>> +}
>> +
>> void
>> nouveau_dmem_fini(struct nouveau_drm *drm)
>> {
>> @@ -380,8 +426,10 @@ nouveau_dmem_fini(struct nouveau_drm *drm)
>> mutex_lock(&drm->dmem->mutex);
>>
>> list_for_each_entry_safe(chunk, tmp, &drm->dmem->chunks, list) {
>> + nouveau_dmem_evict_chunk(chunk);
>> nouveau_bo_unpin(chunk->bo);
>> nouveau_bo_ref(NULL, &chunk->bo);
>> + WARN_ON(chunk->callocated);
>> list_del(&chunk->list);
>> memunmap_pages(&chunk->pagemap);
>> release_mem_region(chunk->pagemap.range.start,
Powered by blists - more mailing lists