| 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: <5f3b6bf9-1762-48b7-bc36-705055c99ce8@nvidia.com>
Date: Fri, 5 Sep 2025 17:56:35 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Alistair Popple <apopple@...dia.com>, linux-mm@...ck.org,
akpm@...ux-foundation.org
Cc: david@...hat.com, osalvador@...e.de, jgg@...pe.ca, peterx@...hat.com,
linux-kernel@...r.kernel.org, dan.j.williams@...el.com,
Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v2 1/2] mm/gup: Remove dead pgmap refcounting code
On 9/3/25 3:59 PM, Alistair Popple wrote:
> Prior to commit aed877c2b425 ("device/dax: properly refcount device dax
> pages when mapping") ZONE_DEVICE pages were not fully reference counted
> when mapped into user page tables. Instead GUP would take a reference
> on the associated pgmap to ensure the results of pfn_to_page() remained
> valid.
>
> This is no longer required and most of the code was removed by commit
> fd2825b0760a ("mm/gup: remove pXX_devmap usage from get_user_pages()").
> Finish cleaning this up by removing the dead calls to put_dev_pagemap()
> and the temporary context struct.
>
> Signed-off-by: Alistair Popple <apopple@...dia.com>
> Acked-by: David Hildenbrand <david@...hat.com>
> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> Reviewed-by: Dan Williams <dan.j.williams@...el.com>
> ---
> mm/gup.c | 67 ++++++++++++++++++++++----------------------------------
> 1 file changed, 26 insertions(+), 41 deletions(-)
>
Wow, nice culmination of a lot of work. So good to remove this
complication!
Maybe I'll post a quick follow-up, in fact, to remove page_mask
as well. No need to pass that around all over the place, I think
it can instead be computed by the single place that actually
consumes it.
Reviewed-by: John Hubbard <jhubbard@...dia.com>
thanks,
--
John Hubbard
> diff --git a/mm/gup.c b/mm/gup.c
> index adffe663594d..be5791a43c73 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -28,11 +28,6 @@
> #include "internal.h"
> #include "swap.h"
>
> -struct follow_page_context {
> - struct dev_pagemap *pgmap;
> - unsigned int page_mask;
> -};
> -
> static inline void sanity_check_pinned_pages(struct page **pages,
> unsigned long npages)
> {
> @@ -661,7 +656,7 @@ static inline bool can_follow_write_pud(pud_t pud, struct page *page,
>
> static struct page *follow_huge_pud(struct vm_area_struct *vma,
> unsigned long addr, pud_t *pudp,
> - int flags, struct follow_page_context *ctx)
> + int flags, unsigned long *page_mask)
> {
> struct mm_struct *mm = vma->vm_mm;
> struct page *page;
> @@ -688,7 +683,7 @@ static struct page *follow_huge_pud(struct vm_area_struct *vma,
> if (ret)
> page = ERR_PTR(ret);
> else
> - ctx->page_mask = HPAGE_PUD_NR - 1;
> + *page_mask = HPAGE_PUD_NR - 1;
>
> return page;
> }
> @@ -714,7 +709,7 @@ static inline bool can_follow_write_pmd(pmd_t pmd, struct page *page,
> static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> unsigned long addr, pmd_t *pmd,
> unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> struct mm_struct *mm = vma->vm_mm;
> pmd_t pmdval = *pmd;
> @@ -751,7 +746,7 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>
> page += (addr & ~HPAGE_PMD_MASK) >> PAGE_SHIFT;
> - ctx->page_mask = HPAGE_PMD_NR - 1;
> + *page_mask = HPAGE_PMD_NR - 1;
>
> return page;
> }
> @@ -759,7 +754,7 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> #else /* CONFIG_PGTABLE_HAS_HUGE_LEAVES */
> static struct page *follow_huge_pud(struct vm_area_struct *vma,
> unsigned long addr, pud_t *pudp,
> - int flags, struct follow_page_context *ctx)
> + int flags, unsigned long *page_mask)
> {
> return NULL;
> }
> @@ -767,7 +762,7 @@ static struct page *follow_huge_pud(struct vm_area_struct *vma,
> static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> unsigned long addr, pmd_t *pmd,
> unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> return NULL;
> }
> @@ -813,8 +808,7 @@ static inline bool can_follow_write_pte(pte_t pte, struct page *page,
> }
>
> static struct page *follow_page_pte(struct vm_area_struct *vma,
> - unsigned long address, pmd_t *pmd, unsigned int flags,
> - struct dev_pagemap **pgmap)
> + unsigned long address, pmd_t *pmd, unsigned int flags)
> {
> struct mm_struct *mm = vma->vm_mm;
> struct folio *folio;
> @@ -912,7 +906,7 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
> static struct page *follow_pmd_mask(struct vm_area_struct *vma,
> unsigned long address, pud_t *pudp,
> unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> pmd_t *pmd, pmdval;
> spinlock_t *ptl;
> @@ -926,7 +920,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
> if (!pmd_present(pmdval))
> return no_page_table(vma, flags, address);
> if (likely(!pmd_leaf(pmdval)))
> - return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
> + return follow_page_pte(vma, address, pmd, flags);
>
> if (pmd_protnone(pmdval) && !gup_can_follow_protnone(vma, flags))
> return no_page_table(vma, flags, address);
> @@ -939,16 +933,16 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
> }
> if (unlikely(!pmd_leaf(pmdval))) {
> spin_unlock(ptl);
> - return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
> + return follow_page_pte(vma, address, pmd, flags);
> }
> if (pmd_trans_huge(pmdval) && (flags & FOLL_SPLIT_PMD)) {
> spin_unlock(ptl);
> split_huge_pmd(vma, pmd, address);
> /* If pmd was left empty, stuff a page table in there quickly */
> return pte_alloc(mm, pmd) ? ERR_PTR(-ENOMEM) :
> - follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
> + follow_page_pte(vma, address, pmd, flags);
> }
> - page = follow_huge_pmd(vma, address, pmd, flags, ctx);
> + page = follow_huge_pmd(vma, address, pmd, flags, page_mask);
> spin_unlock(ptl);
> return page;
> }
> @@ -956,7 +950,7 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
> static struct page *follow_pud_mask(struct vm_area_struct *vma,
> unsigned long address, p4d_t *p4dp,
> unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> pud_t *pudp, pud;
> spinlock_t *ptl;
> @@ -969,7 +963,7 @@ static struct page *follow_pud_mask(struct vm_area_struct *vma,
> return no_page_table(vma, flags, address);
> if (pud_leaf(pud)) {
> ptl = pud_lock(mm, pudp);
> - page = follow_huge_pud(vma, address, pudp, flags, ctx);
> + page = follow_huge_pud(vma, address, pudp, flags, page_mask);
> spin_unlock(ptl);
> if (page)
> return page;
> @@ -978,13 +972,13 @@ static struct page *follow_pud_mask(struct vm_area_struct *vma,
> if (unlikely(pud_bad(pud)))
> return no_page_table(vma, flags, address);
>
> - return follow_pmd_mask(vma, address, pudp, flags, ctx);
> + return follow_pmd_mask(vma, address, pudp, flags, page_mask);
> }
>
> static struct page *follow_p4d_mask(struct vm_area_struct *vma,
> unsigned long address, pgd_t *pgdp,
> unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> p4d_t *p4dp, p4d;
>
> @@ -995,7 +989,7 @@ static struct page *follow_p4d_mask(struct vm_area_struct *vma,
> if (!p4d_present(p4d) || p4d_bad(p4d))
> return no_page_table(vma, flags, address);
>
> - return follow_pud_mask(vma, address, p4dp, flags, ctx);
> + return follow_pud_mask(vma, address, p4dp, flags, page_mask);
> }
>
> /**
> @@ -1003,20 +997,16 @@ static struct page *follow_p4d_mask(struct vm_area_struct *vma,
> * @vma: vm_area_struct mapping @address
> * @address: virtual address to look up
> * @flags: flags modifying lookup behaviour
> - * @ctx: contains dev_pagemap for %ZONE_DEVICE memory pinning and a
> - * pointer to output page_mask
> + * @page_mask: a pointer to output page_mask
> *
> * @flags can have FOLL_ flags set, defined in <linux/mm.h>
> *
> - * When getting pages from ZONE_DEVICE memory, the @ctx->pgmap caches
> - * the device's dev_pagemap metadata to avoid repeating expensive lookups.
> - *
> * When getting an anonymous page and the caller has to trigger unsharing
> * of a shared anonymous page first, -EMLINK is returned. The caller should
> * trigger a fault with FAULT_FLAG_UNSHARE set. Note that unsharing is only
> * relevant with FOLL_PIN and !FOLL_WRITE.
> *
> - * On output, the @ctx->page_mask is set according to the size of the page.
> + * On output, @page_mask is set according to the size of the page.
> *
> * Return: the mapped (struct page *), %NULL if no mapping exists, or
> * an error pointer if there is a mapping to something not represented
> @@ -1024,7 +1014,7 @@ static struct page *follow_p4d_mask(struct vm_area_struct *vma,
> */
> static struct page *follow_page_mask(struct vm_area_struct *vma,
> unsigned long address, unsigned int flags,
> - struct follow_page_context *ctx)
> + unsigned long *page_mask)
> {
> pgd_t *pgd;
> struct mm_struct *mm = vma->vm_mm;
> @@ -1032,13 +1022,13 @@ static struct page *follow_page_mask(struct vm_area_struct *vma,
>
> vma_pgtable_walk_begin(vma);
>
> - ctx->page_mask = 0;
> + *page_mask = 0;
> pgd = pgd_offset(mm, address);
>
> if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
> page = no_page_table(vma, flags, address);
> else
> - page = follow_p4d_mask(vma, address, pgd, flags, ctx);
> + page = follow_p4d_mask(vma, address, pgd, flags, page_mask);
>
> vma_pgtable_walk_end(vma);
>
> @@ -1376,7 +1366,7 @@ static long __get_user_pages(struct mm_struct *mm,
> {
> long ret = 0, i = 0;
> struct vm_area_struct *vma = NULL;
> - struct follow_page_context ctx = { NULL };
> + unsigned long page_mask = 0;
>
> if (!nr_pages)
> return 0;
> @@ -1418,7 +1408,7 @@ static long __get_user_pages(struct mm_struct *mm,
> pages ? &page : NULL);
> if (ret)
> goto out;
> - ctx.page_mask = 0;
> + page_mask = 0;
> goto next_page;
> }
>
> @@ -1441,7 +1431,7 @@ static long __get_user_pages(struct mm_struct *mm,
> }
> cond_resched();
>
> - page = follow_page_mask(vma, start, gup_flags, &ctx);
> + page = follow_page_mask(vma, start, gup_flags, &page_mask);
> if (!page || PTR_ERR(page) == -EMLINK) {
> ret = faultin_page(vma, start, gup_flags,
> PTR_ERR(page) == -EMLINK, locked);
> @@ -1474,7 +1464,7 @@ static long __get_user_pages(struct mm_struct *mm,
> goto out;
> }
> next_page:
> - page_increm = 1 + (~(start >> PAGE_SHIFT) & ctx.page_mask);
> + page_increm = 1 + (~(start >> PAGE_SHIFT) & page_mask);
> if (page_increm > nr_pages)
> page_increm = nr_pages;
>
> @@ -1524,8 +1514,6 @@ static long __get_user_pages(struct mm_struct *mm,
> nr_pages -= page_increm;
> } while (nr_pages);
> out:
> - if (ctx.pgmap)
> - put_dev_pagemap(ctx.pgmap);
> return i ? i : ret;
> }
>
> @@ -2853,7 +2841,6 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
> unsigned long end, unsigned int flags, struct page **pages,
> int *nr)
> {
> - struct dev_pagemap *pgmap = NULL;
> int ret = 0;
> pte_t *ptep, *ptem;
>
> @@ -2926,8 +2913,6 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
> ret = 1;
>
> pte_unmap:
> - if (pgmap)
> - put_dev_pagemap(pgmap);
> pte_unmap(ptem);
> return ret;
> }
Powered by blists - more mailing lists