Re: [PATCH v2] mm: implement and use vma_anon_tracked(), silence KCSAN

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Sep 10, 2026 at 04:02:43PM +0200, David Hildenbrand (Arm) wrote:
> On 9/10/26 15:18, Lorenzo Stoakes (ARM) wrote:
> > Provide a function to abstract the common task of checking whether
> > a VMA is faulted in or not.
> >
> > A VMA or mmap lock must be held when calling this function. For an attached
> > VMA the transitions between unfaulted/faulted state are:
> >
> >        Transition        |              VMA/mmap Lock state
> >  ------------------------|-----------------------------------------------
> >    unfaulted to faulted  | write lock OR read lock + mm->page_table_lock
> >    faulted to unfaulted  | write lock
> >
>
> Still using the faulted/unfaulted terminology above is subotimal but I don't
> mind too much as long as the code is easy to grasp :)

I think this would read terribly as 'anon rmap untracked/anon rmap tracked'
really.

This is why I like fault as a verb here, it is explicitly the event that
changes state, and also as a noun because once faulted, a VMA is faulted
and it's just how I've thought about this for years now :)

I understand, obviously, that VMA_SHARED_BIT mappings can be faulted
unrelatedly but vma_anon_faulted() would then be my preference rather than
abstracting that (yes I know there are some weird edge cases on with that
state too).

But I am compromising on the name :)

However as far as description goes, I think it's far clearer this way.

I'm happy to hear an alternative if you feel something else would be
clearer here though!

>
> > So vma_anon_tracked() never provides a false positive (the lock precludes
> > it), but if only a read lock is held, a negative result must be re-checked
> > with mm->page_table_lock held.
> >
> > Detached VMAs cannot be concurrently manipulated as they are removed from
> > the maple tree so require no guarantees.
> >
> > Use data_race() to silence KCSAN about non-existent data races between
> > concurrent vma->anon_vma read/write on optimistic fault tests.
> >
> > Also while here, const-ify vma_is_attached(), vma_assert_stabilised() and
> > dependants.
> >
> > Finally, update the core VMA merge/split, rmap, mremap, KSM and fault
> > preparation callers which test vma->anon_vma directly to use
> > vma_anon_tracked() instead.
> >
> > Note that the lockless read in reusable_anon_vma() is doing more than
> > checking whether the VMA is faulted - it is returning the anon_vma to be
> > used on fault, so this check is not altered.
> >
> > There is one odd one out - file_backed_vma_is_retractable() - which holds
> > neither a VMA nor mmap lock and is stabilised by the file rmap lock only.
> >
> > Therefore just add a comment to explain why the direct vma->anon_vma check
> > is required.
> >
> > Reported-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> > Closes: https://lore.kernel.org/all/20260829100034.423064-1-trintaeoitogc@gmail.com/
> > Closes: https://lore.kernel.org/all/20260909115723.528501-1-trintaeoitogc@gmail.com/
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > ---
> > v2:
> > - Renamed vma_is_faulted() to vma_anon_tracked() as per David.
> >
> > v1:
> > https://lore.kernel.org/r/20260909-vma-is-faulted-v1-1-3a701f48984a@kernel.org
> > ---
>
>
>
> > index dd66c6ad5af1..40669bec953c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -264,7 +264,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
> >  	 * Allow page fault since anon_vma may be not initialized until
> >  	 * the first page fault.
>
> Do we directly want to fix up such comments to not talk about "anon_vma" but
> instead of the abstraction?
>
> Not immediately sure how to rephrase, but I guess the goal would be to avoid the
> term "anon_vma" and least outside of core-rmap/anon_vma code.

I'll do a pass through. Can be a separate commit in the series I guess.

>
> [...]
>
> >  		/* KSM folios don't reach here because of the !anon_vma check */
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 01e7b6b046b6..9f9cd6657360 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -2705,7 +2705,7 @@ static int unuse_mm(struct mm_struct *mm, unsigned int type)
> >  	if (check_stable_address_space(mm))
> >  		goto unlock;
> >  	for_each_vma(vmi, vma) {
> > -		if (vma->anon_vma && !is_vm_hugetlb_page(vma)) {
> > +		if (vma_anon_tracked(vma) && !is_vm_hugetlb_page(vma)) {
> >  			ret = unuse_vma(vma, type);
> >  			if (ret)
> >  				break;
>
> [...]
>
> > +/**
> > + * vma_anon_tracked() - is @vma tracked by an anon_vma?
> > + * @vma: The VMA to be checked.
> > + *
> > + * A VMA or mmap lock must be held.
>
> I would document here what the implication is: if untracked, there cannot be any
> anon folios, if tracked, there might. Some callers of this function (e.g., ksm,
> swapfile) want to know exactly that: could there be any anon folio in there such
> that they have to provess the VMA.

Process?

I'll add some more details to the kdoc.

I do think the name is poor now on reflection, vma_has_anon_rmap() seems
stronger. You OK with that?

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo