On Wed, Aug 05, 2026 at 09:35:56AM +0200, David Hildenbrand (Arm) wrote:
> On 8/3/26 15:46, Lorenzo Stoakes (ARM) wrote:
> > On Mon, Aug 03, 2026 at 12:52:42PM +0200, David Hildenbrand (Arm) wrote:
> >> On 7/29/26 18:48, Lorenzo Stoakes (ARM) wrote:
> >>> We must correctly update VMA anonymous page offset state on all VMA
> >>> operations that would result in it changing, with special attention given
> >>> to remapping.
> >>>
> >>> We cover most cases by simply updating vma_set_range() to do so (with a
> >>> new
> >>> anonymous page offset parameter), but also notably must update the merging
> >>> and mapping logic to propagate this parameter correctly.
> >>>
> >>> The remap logic remains the same - we may update the anonymous page offset
> >>> if the VMA is unfaulted, but now this applies to MAP_PRIVATE file-backed
> >>> mappings too, so we update the code to reflect this.
> >>>
> >>> Note that we use __linear_anon_page_index() upon remap as the VMA may be
> >>> shared, in order that we update the field consistently regardless of VMA
> >>> type.
> >>>
> >>> Similarly, pass through anon page offset to the merge logic, updating the
> >>> vma_merge_struct struct to propagate it, and also use
> >>> __linear_anon_page_index() to obtain the anonymous page index so it can be
> >>> safely used for both shared and MAP_PRIVATE file-backed mappings.
> >>>
> >>> Finally, we update insert_vm_struct() to correctly set the anonymous page
> >>> offset on insertion of a VMA.
> >>>
> >>> We simply ensure state is correctly propagated here, so no functional
> >>> changes are intended.
> >>>
> >>> Also while we're here, replace a VM_BUG_ON_VMA() with a
> >>> VM_WARN_ON_ONCE_VMA().
> >>>
> >>> Also update VMA userland tests to reflect this change.
> >>>
> >>> Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
> >>
> >>
> >> [...]
> >>
> >>> struct vm_area_struct *vma = *vmap;
> >>> unsigned long vma_start = vma->vm_start;
> >>> @@ -1919,11 +1929,14 @@ struct vm_area_struct *copy_vma(struct
> >>> vm_area_struct **vmap,
> >>> VMG_VMA_STATE(vmg, &vmi, NULL, vma, addr, addr + len);
> >>>
> >>> /*
> >>> - * If anonymous vma has not yet been faulted, update new pgoff
> >>> - * to match new location, to increase its chance of merging.
> >>> + * If a vma has not yet been faulted, update its anonymous pgoff to
> >>> + * match the new location to increase its chance of merging.
> >>> */
> >>> - if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) {
> >>> - pgoff = addr >> PAGE_SHIFT;
> >>> + if (!vma->anon_vma && !vma_test(vma, VMA_SHARED_BIT)) {
> >>
> >> Could we also use is_cow_mapping() ?
> >
> > No this would be incorrect.
> >
> > A read-only mapping would become unmergeable here. So this is something
> > apart
> > from the rmap aspect,
>
> I'd assume that we should never even consider anon_pgoff when merging
> !is_cow_mapping(), it doesn't make any sense.
>
> No anon folios -> no anon_vma -> no anon_pgoff
You can merge unfaulted ranges is the thing here.
But anyway I actually wonder whether this whole branch shouldn't be:
if (!vma->anon_vma) {
...
}
Because that way we keep anon_pgoff updated even for MAP_SHARED mappings. This
isn't necessary and doesn't impact anything _except_ print_bad_page_map which
outputs both pgoffs.
But it'd be consistent, avoid any confusion about gating on VMA_SHARED, and
simplify the code :)
>
> But I think I am missing one detail here:
>
> > and it is a contract that upon move of an unfaulted
> > mapping (which for read-only anon would always be unfaulted) that
> > vma->vm_pgoff
> > is updated.
>
> "read-only anon": I assume you mean an anon mapping that does not have
> VM_MAYWRITE set?
A MAP_SHARED mapping of a read-only file becomes a MAP_PRIVATE !VMA_MAYWRITE_BIT
mapping and must adhere to the same contract.
Also mmap hooks can clear the VMA_MAYWRITE_BIT.
However:
- If you're a driver clearing VMA_MAYWRITE_BIT you should only be doing this for
'special' mappings anyway (I have a series I've not sent yet that establishes
this as an invariant also) - and these are not mergeable anyway.
- If you're a !VMA_MAYWRITE_BIT MAP_PRIVATE-file backed mappings you never set
vma->anon_vma and always update anon pgoff so you always have alignment for
purposes of merge.
So I think also we can then change needs_adjacent_anon_pgoff() to:
static bool needs_adjacent_anon_pgoff(const struct vma_merge_struct *vmg)
{
return vmg->file && is_cow_mapping(...);
}
[I have to create a vma_flags_t variant of is_cow_mapping()]
With those two changes we gate on VMA_SHARED_BIT nowhere :)
>
> I recall that that's a combination that cannot be created. While you can
> create
> something that does not have VM_WRITE set, IIRC VM_MAYWRITE is always set for
> anon vmas.
For pure anon yeah, see above for the MAP_SHARED->MAP_PRIVATE-file backed weird
case.
>
> --
> Cheers,
>
> David
(It's funny to me that if you want a truly read-only MAP_PRIVATE file-backed
mapping (no idea why you would but anyway) you have to MAP_SHARED, but an
actually MAP_PRIVATE file-backed mapping of a read-only file is writable [which
makes sense obviously] :)
--
Cheers, Lorenzo
