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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 14 Jul 2022 15:31:32 +1000 From: Alistair Popple <apopple@...dia.com> To: David Hildenbrand <david@...hat.com> Cc: Alex Sierra <alex.sierra@....com>, jgg@...dia.com, Felix.Kuehling@....com, linux-mm@...ck.org, rcampbell@...dia.com, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, hch@....de, jglisse@...hat.com, willy@...radead.org, akpm@...ux-foundation.org Subject: Re: [PATCH v8 06/15] mm: remove the vma check in migrate_vma_setup() David Hildenbrand <david@...hat.com> writes: > On 07.07.22 21:03, Alex Sierra wrote: >> From: Alistair Popple <apopple@...dia.com> >> >> migrate_vma_setup() checks that a valid vma is passed so that the page >> tables can be walked to find the pfns associated with a given address >> range. However in some cases the pfns are already known, such as when >> migrating device coherent pages during pin_user_pages() meaning a valid >> vma isn't required. > > As raised in my other reply, without a VMA ... it feels odd to use a > "migrate_vma" API. For an internal (mm/migrate_device.c) use case it is > ok I guess, but it certainly adds a bit of confusion. For example, > because migrate_vma_setup() will undo ref+lock not obtained by it. > > I guess the interesting point is that > > a) Besides migrate_vma_pages() and migrate_vma_setup(), the ->vma is unused. > > b) migrate_vma_setup() does collect+unmap+cleanup if unmap failed. > > c) With our source page in our hands, we cannot be processing a hole in > a VMA. > > > > Not sure if it's better. but I would > > a) Enforce in migrate_vma_setup() that there is a VMA. Code outside of > mm/migrate_device.c shouldn't be doing some hacks like this. > > b) Don't call migrate_vma_setup() from migrate_device_page(), but > directly migrate_vma_unmap() and add a comment. > > > That will leave a single change to this patch (migrate_vma_pages()). But > is that even required? Because .... > >> @@ -685,7 +685,7 @@ void migrate_vma_pages(struct migrate_vma *migrate) >> continue; >> } >> >> - if (!page) { >> + if (!page && migrate->vma) { > > How could we ever have !page in case of migrate_device_page()? Oh good point. This patch was originally part of a larger series I was working on at the time but you're right - for migrate_device_page() we should never hit this case. I will respin the next patch (number 7 in this series) to include this. > Instead, I think a VM_BUG_ON(migrate->vma); should hold and you can just > simplify. > >> if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE)) >> continue; >> if (!notified) {
Powered by blists - more mailing lists