Message-ID: <8d96176d-f36b-4714-9780-6131c8470f50@lucifer.local>
Date: Tue, 13 May 2025 11:16:16 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-trace-kernel@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>, "H. Peter Anvin" <hpa@...or.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tursulin@...ulin.net>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, Peter Xu <peterx@...hat.com>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v2 04/11] mm: convert VM_PFNMAP tracking to
 pfnmap_track() + pfnmap_untrack()

On Tue, May 13, 2025 at 11:10:45AM +0200, David Hildenbrand wrote:
> On 12.05.25 18:42, Lorenzo Stoakes wrote:
> > On Mon, May 12, 2025 at 02:34:17PM +0200, David Hildenbrand wrote:
> > > Let's use our new interface. In remap_pfn_range(), we'll now decide
> > > whether we have to track (full VMA covered) or only lookup the
> > > cachemode (partial VMA covered).
> > >
> > > Remember what we have to untrack by linking it from the VMA. When
> > > duplicating VMAs (e.g., splitting, mremap, fork), we'll handle it similar
> > > to anon VMA names, and use a kref to share the tracking.
> > >
> > > Once the last VMA un-refs our tracking data, we'll do the untracking,
> > > which simplifies things a lot and should sort our various issues we saw
> > > recently, for example, when partially unmapping/zapping a tracked VMA.
> > >
> > > This change implies that we'll keep tracking the original PFN range even
> > > after splitting + partially unmapping it: not too bad, because it was
> > > not working reliably before. The only thing that kind-of worked before
> > > was shrinking such a mapping using mremap(): we managed to adjust the
> > > reservation in a hacky way, now we won't adjust the reservation but
> > > leave it around until all involved VMAs are gone.
> > >
> > > If that ever turns out to be an issue, we could hook into VM splitting
> > > code and split the tracking; however, that adds complexity that might
> > > not be required, so we'll keep it simple for now.
> > >
> > > Acked-by: Ingo Molnar <mingo@...nel.org> # x86 bits
> > > Signed-off-by: David Hildenbrand <david@...hat.com>
> >
> > Other than small nit below,
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> >
> > > ---
> > >   include/linux/mm_inline.h |  2 +
> > >   include/linux/mm_types.h  | 11 ++++++
> > >   mm/memory.c               | 82 +++++++++++++++++++++++++++++++--------
> > >   mm/mmap.c                 |  5 ---
> > >   mm/mremap.c               |  4 --
> > >   mm/vma_init.c             | 50 ++++++++++++++++++++++++
> > >   6 files changed, 129 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> > > index f9157a0c42a5c..89b518ff097e6 100644
> > > --- a/include/linux/mm_inline.h
> > > +++ b/include/linux/mm_inline.h
> > > @@ -447,6 +447,8 @@ static inline bool anon_vma_name_eq(struct anon_vma_name *anon_name1,
> > >
> > >   #endif  /* CONFIG_ANON_VMA_NAME */
> > >
> > > +void pfnmap_track_ctx_release(struct kref *ref);
> > > +
> > >   static inline void init_tlb_flush_pending(struct mm_struct *mm)
> > >   {
> > >   	atomic_set(&mm->tlb_flush_pending, 0);
> > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > > index 15808cad2bc1a..3e934dc6057c4 100644
> > > --- a/include/linux/mm_types.h
> > > +++ b/include/linux/mm_types.h
> > > @@ -763,6 +763,14 @@ struct vma_numab_state {
> > >   	int prev_scan_seq;
> > >   };
> > >
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > > +struct pfnmap_track_ctx {
> > > +	struct kref kref;
> > > +	unsigned long pfn;
> > > +	unsigned long size;	/* in bytes */
> > > +};
> > > +#endif
> > > +
> > >   /*
> > >    * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
> > >    * manipulate mutable fields which will cause those fields to be updated in the
> > > @@ -900,6 +908,9 @@ struct vm_area_struct {
> > >   	struct anon_vma_name *anon_name;
> > >   #endif
> > >   	struct vm_userfaultfd_ctx vm_userfaultfd_ctx;
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > > +	struct pfnmap_track_ctx *pfnmap_track_ctx;
> > > +#endif
> > >   } __randomize_layout;
> > >
> > >   #ifdef CONFIG_NUMA
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 064fc55d8eab9..4cf4adb0de266 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -1371,7 +1371,7 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
> > >   	struct mm_struct *dst_mm = dst_vma->vm_mm;
> > >   	struct mm_struct *src_mm = src_vma->vm_mm;
> > >   	struct mmu_notifier_range range;
> > > -	unsigned long next, pfn = 0;
> > > +	unsigned long next;
> > >   	bool is_cow;
> > >   	int ret;
> > >
> > > @@ -1381,12 +1381,6 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
> > >   	if (is_vm_hugetlb_page(src_vma))
> > >   		return copy_hugetlb_page_range(dst_mm, src_mm, dst_vma, src_vma);
> > >
> > > -	if (unlikely(src_vma->vm_flags & VM_PFNMAP)) {
> > > -		ret = track_pfn_copy(dst_vma, src_vma, &pfn);
> > > -		if (ret)
> > > -			return ret;
> > > -	}
> > > -
> > >   	/*
> > >   	 * We need to invalidate the secondary MMU mappings only when
> > >   	 * there could be a permission downgrade on the ptes of the
> > > @@ -1428,8 +1422,6 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
> > >   		raw_write_seqcount_end(&src_mm->write_protect_seq);
> > >   		mmu_notifier_invalidate_range_end(&range);
> > >   	}
> > > -	if (ret && unlikely(src_vma->vm_flags & VM_PFNMAP))
> > > -		untrack_pfn_copy(dst_vma, pfn);
> > >   	return ret;
> > >   }
> > >
> > > @@ -1924,9 +1916,6 @@ static void unmap_single_vma(struct mmu_gather *tlb,
> > >   	if (vma->vm_file)
> > >   		uprobe_munmap(vma, start, end);
> > >
> > > -	if (unlikely(vma->vm_flags & VM_PFNMAP))
> > > -		untrack_pfn(vma, 0, 0, mm_wr_locked);
> > > -
> > >   	if (start != end) {
> > >   		if (unlikely(is_vm_hugetlb_page(vma))) {
> > >   			/*
> > > @@ -2872,6 +2861,36 @@ int remap_pfn_range_notrack(struct vm_area_struct *vma, unsigned long addr,
> > >   	return error;
> > >   }
> > >
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > > +static inline struct pfnmap_track_ctx *pfnmap_track_ctx_alloc(unsigned long pfn,
> > > +		unsigned long size, pgprot_t *prot)
> > > +{
> > > +	struct pfnmap_track_ctx *ctx;
> > > +
> > > +	if (pfnmap_track(pfn, size, prot))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> > > +	if (unlikely(!ctx)) {
> > > +		pfnmap_untrack(pfn, size);
> > > +		return ERR_PTR(-ENOMEM);
> > > +	}
> > > +
> > > +	ctx->pfn = pfn;
> > > +	ctx->size = size;
> > > +	kref_init(&ctx->kref);
> > > +	return ctx;
> > > +}
> > > +
> > > +void pfnmap_track_ctx_release(struct kref *ref)
> > > +{
> > > +	struct pfnmap_track_ctx *ctx = container_of(ref, struct pfnmap_track_ctx, kref);
> > > +
> > > +	pfnmap_untrack(ctx->pfn, ctx->size);
> > > +	kfree(ctx);
> > > +}
> > > +#endif /* __HAVE_PFNMAP_TRACKING */
> > > +
> > >   /**
> > >    * remap_pfn_range - remap kernel memory to userspace
> > >    * @vma: user vma to map to
> > > @@ -2884,20 +2903,51 @@ int remap_pfn_range_notrack(struct vm_area_struct *vma, unsigned long addr,
> > >    *
> > >    * Return: %0 on success, negative error code otherwise.
> > >    */
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > >   int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr,
> > >   		    unsigned long pfn, unsigned long size, pgprot_t prot)
> > >   {
> > > +	struct pfnmap_track_ctx *ctx = NULL;
> > >   	int err;
> > >
> > > -	err = track_pfn_remap(vma, &prot, pfn, addr, PAGE_ALIGN(size));
> > > -	if (err)
> > > +	size = PAGE_ALIGN(size);
> > > +
> > > +	/*
> > > +	 * If we cover the full VMA, we'll perform actual tracking, and
> > > +	 * remember to untrack when the last reference to our tracking
> > > +	 * context from a VMA goes away. We'll keep tracking the whole pfn
> > > +	 * range even during VMA splits and partial unmapping.
> > > +	 *
> > > +	 * If we only cover parts of the VMA, we'll only setup the cachemode
> > > +	 * in the pgprot for the pfn range.
> > > +	 */
> > > +	if (addr == vma->vm_start && addr + size == vma->vm_end) {
> > > +		if (vma->pfnmap_track_ctx)
> > > +			return -EINVAL;
> > > +		ctx = pfnmap_track_ctx_alloc(pfn, size, &prot);
> > > +		if (IS_ERR(ctx))
> > > +			return PTR_ERR(ctx);
> > > +	} else if (pfnmap_setup_cachemode(pfn, size, &prot)) {
> > >   		return -EINVAL;
> > > +	}
> > >
> > >   	err = remap_pfn_range_notrack(vma, addr, pfn, size, prot);
> > > -	if (err)
> > > -		untrack_pfn(vma, pfn, PAGE_ALIGN(size), true);
> > > +	if (ctx) {
> > > +		if (err)
> > > +			kref_put(&ctx->kref, pfnmap_track_ctx_release);
> > > +		else
> > > +			vma->pfnmap_track_ctx = ctx;
> > > +	}
> > >   	return err;
> > >   }
> > > +
> > > +#else
> > > +int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr,
> > > +		    unsigned long pfn, unsigned long size, pgprot_t prot)
> > > +{
> > > +	return remap_pfn_range_notrack(vma, addr, pfn, size, prot);
> > > +}
> > > +#endif
> > >   EXPORT_SYMBOL(remap_pfn_range);
> > >
> > >   /**
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 50f902c08341a..09c563c951123 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -1784,11 +1784,6 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
> > >   		tmp = vm_area_dup(mpnt);
> > >   		if (!tmp)
> > >   			goto fail_nomem;
> > > -
> > > -		/* track_pfn_copy() will later take care of copying internal state. */
> > > -		if (unlikely(tmp->vm_flags & VM_PFNMAP))
> > > -			untrack_pfn_clear(tmp);
> > > -
> > >   		retval = vma_dup_policy(mpnt, tmp);
> > >   		if (retval)
> > >   			goto fail_nomem_policy;
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index 7db9da609c84f..6e78e02f74bd3 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -1191,10 +1191,6 @@ static int copy_vma_and_data(struct vma_remap_struct *vrm,
> > >   	if (is_vm_hugetlb_page(vma))
> > >   		clear_vma_resv_huge_pages(vma);
> > >
> > > -	/* Tell pfnmap has moved from this vma */
> > > -	if (unlikely(vma->vm_flags & VM_PFNMAP))
> > > -		untrack_pfn_clear(vma);
> > > -
> > >   	*new_vma_ptr = new_vma;
> > >   	return err;
> > >   }
> > > diff --git a/mm/vma_init.c b/mm/vma_init.c
> > > index 967ca85179864..8e53c7943561e 100644
> > > --- a/mm/vma_init.c
> > > +++ b/mm/vma_init.c
> > > @@ -71,7 +71,51 @@ static void vm_area_init_from(const struct vm_area_struct *src,
> > >   #ifdef CONFIG_NUMA
> > >   	dest->vm_policy = src->vm_policy;
> > >   #endif
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > > +	dest->pfnmap_track_ctx = NULL;
> > > +#endif
> > > +}
> > > +
> > > +#ifdef __HAVE_PFNMAP_TRACKING
> > > +static inline int vma_pfnmap_track_ctx_dup(struct vm_area_struct *orig,
> > > +		struct vm_area_struct *new)
> > > +{
> > > +	struct pfnmap_track_ctx *ctx = orig->pfnmap_track_ctx;
> > > +
> > > +	if (likely(!ctx))
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * We don't expect to ever hit this. If ever required, we would have
> > > +	 * to duplicate the tracking.
> > > +	 */
> > > +	if (unlikely(kref_read(&ctx->kref) >= REFCOUNT_MAX))
> >
> > How not expected is this? :) maybe use WARN_ON_ONCE() if it really should
> > never happen?
> I guess if we mmap a large PFNMAP and then split it into individual
> PTE-sized chunks, we could get many VMAs per-process referencing that
> tracing.
>
> Combine that with fork() and I assume one could hit this -- when really
> trying hard to achieve it. (probably as privileged user to get a big
> VM_PFNMAP, though, but not sure)

Right ok, yeah I guess so. It'd be good to see if we could trigger it somehow :)

>
> In that case, a WARN_ON_ONCE() would be bad -- because it could be triggered
> by the user.

Ack

>
> We could do a pr_warn_once() instead, stating that this is not supported
> right now?

Hmm, if we truly think it might happen let's avoid printing anything for now.

Maybe just ++todo for experimenting with triggering?

It's not hugely important!

>
> --
> Cheers,
>
> David / dhildenb
>

Powered by blists - more mailing lists