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 Oct 2021 10:35:27 -0700 From: Ralph Campbell <rcampbell@...dia.com> To: Jason Gunthorpe <jgg@...dia.com>, Alex Sierra <alex.sierra@....com> CC: <akpm@...ux-foundation.org>, <Felix.Kuehling@....com>, <linux-mm@...ck.org>, <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>, <apopple@...dia.com>, <willy@...radead.org> Subject: Re: [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct page refcount On 10/14/21 10:06 AM, Jason Gunthorpe wrote: > On Thu, Oct 14, 2021 at 10:39:28AM -0500, Alex Sierra wrote: >> From: Ralph Campbell <rcampbell@...dia.com> >> >> ZONE_DEVICE struct pages have an extra reference count that complicates the >> code for put_page() and several places in the kernel that need to check the >> reference count to see that a page is not being used (gup, compaction, >> migration, etc.). Clean up the code so the reference count doesn't need to >> be treated specially for ZONE_DEVICE. >> >> Signed-off-by: Ralph Campbell <rcampbell@...dia.com> >> Signed-off-by: Alex Sierra <alex.sierra@....com> >> Reviewed-by: Christoph Hellwig <hch@....de> >> --- >> v2: >> AS: merged this patch in linux 5.11 version >> >> v5: >> AS: add condition at try_grab_page to check for the zone device type, while >> page ref counter is checked less/equal to zero. In case of device zone, pages >> ref counter are initialized to zero. >> >> v7: >> AS: fix condition at try_grab_page added at v5, is invalid. It supposed >> to fix xfstests/generic/413 test, however, there's a known issue on >> this test where DAX mapped area DIO to non-DAX expect to fail. >> https://patchwork.kernel.org/project/fstests/patch/1489463960-3579-1-git-send-email-xzhou@redhat.com >> This condition was removed after rebase over patch series >> https://lore.kernel.org/r/20210813044133.1536842-4-jhubbard@nvidia.com >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 2 +- >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +- >> fs/dax.c | 4 +- >> include/linux/dax.h | 2 +- >> include/linux/memremap.h | 7 +-- >> include/linux/mm.h | 11 ---- >> lib/test_hmm.c | 2 +- >> mm/internal.h | 8 +++ >> mm/memcontrol.c | 6 +-- >> mm/memremap.c | 69 +++++++------------------- >> mm/migrate.c | 5 -- >> mm/page_alloc.c | 3 ++ >> mm/swap.c | 45 ++--------------- >> 13 files changed, 46 insertions(+), 120 deletions(-) > Has anyone tested this with FSDAX? Does get_user_pages() on fsdax > backed memory still work? I ran xfstests-dev using the kernel boot option to "fake" a pmem device when I first posted this patch. The tests ran OK (or at least the same tests passed with and without my patch). However, I could never really convince myself the changes were "OK" for fsdax since I didn't understand the code that well. I would still like to see a xfsdax maintainer or expert ACK this change. https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git > What refcount value does the struct pages have when they are installed > in the PTEs? Remember a 0 refcount will make all the get_user_pages() > fail. > > I'm looking at the call path starting in ext4_punch_hole() and I would > expect to see something manipulating the page ref count before > the ext4_break_layouts() call path gets to the dax_page_unused() test. > > All I see is we go into unmap_mapping_pages() - that would normally > put back the page references held by PTEs but insert_pfn() has this: > > if (pfn_t_devmap(pfn)) > entry = pte_mkdevmap(pfn_t_pte(pfn, prot)); > > And: > > static inline pte_t pte_mkdevmap(pte_t pte) > { > return pte_set_flags(pte, _PAGE_SPECIAL|_PAGE_DEVMAP); > } > > Which interacts with vm_normal_page(): > > if (pte_devmap(pte)) > return NULL; > > To disable that refcounting? > > So... I have a feeling this will have PTEs pointing to 0 refcount > pages? Unless FSDAX is !pte_devmap which is not the case, right? > > This seems further confirmed by this comment: > > /* > * If we race get_user_pages_fast() here either we'll see the > * elevated page count in the iteration and wait, or > * get_user_pages_fast() will see that the page it took a reference > * against is no longer mapped in the page tables and bail to the > * get_user_pages() slow path. The slow path is protected by > * pte_lock() and pmd_lock(). New references are not taken without > * holding those locks, and unmap_mapping_pages() will not zero the > * pte or pmd without holding the respective lock, so we are > * guaranteed to either see new references or prevent new > * references from being established. > */ > > Which seems to explain this scheme relies on unmap_mapping_pages() to > fence GUP_fast, not on GUP_fast observing 0 refcounts when it should > stop. > > This seems like it would be properly fixed by using normal page > refcounting for PTEs - ie stop using special for these pages? > > Does anyone know why devmap is pte_special anyhow? > >> +void free_zone_device_page(struct page *page) >> +{ >> + switch (page->pgmap->type) { >> + case MEMORY_DEVICE_PRIVATE: >> + free_device_page(page); >> + return; >> + case MEMORY_DEVICE_FS_DAX: >> + /* notify page idle */ >> + wake_up_var(&page->_refcount); >> + return; > It is not for this series, but I wonder if we should just always call > ops->page_free and have free_device_page() logic in that callback for > the non-fs-dax cases? > > For instance where is the mem_cgroup_charge() call to pair with the > mem_cgroup_uncharge() in free_device_page()? > > Isn't cgroup charging (or not) the responsibility of the "allocator" > eg the pgmap_ops owner? > > Jason
Powered by blists - more mailing lists