On Wed, Sep 23, 2026 at 09:09:38AM -0700, Suren Baghdasaryan wrote: > On Wed, Sep 23, 2026 at 8:54 AM Lorenzo Stoakes (ARM) <[email protected]> wrote: > > > > On Wed, Sep 23, 2026 at 08:32:43AM -0700, Suren Baghdasaryan wrote: > > > On Thu, Sep 17, 2026 at 9:24 AM Lorenzo Stoakes (ARM) <[email protected]> > > > wrote: > > > > > > > > It only makes sense to manipulate VMA fields if we allocated a new VMA, > > > > rather than merged it. > > > > > > > > VMA merging does not compare vm_ops or vm_private_data, so a merged VMA > > > > keeps its own, which is also what the legacy f_op->mmap path does since > > > > it > > > > never touches an existing VMA. Previously set_vma_user_defined_fields() > > > > overwrote the merged VMA's fields with those set for the new mapping. In > > > > practice these are the same values, with rare exceptions such as shmem > > > > selecting vm_ops based on whether the file has been unlinked, so no > > > > user-visible change is expected. > > > > > > > > Make this dependency explicit, and additionally constify > > > > have_mmap_prepare > > > > while we're here. > > > > > > The fact that we might be overriding attributes of an existing VMA > > > that we merged with is technically a bug even if we never hit it, > > > right? If so, should we have: > > > > > > Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback") > > > > It's not a bug, it is an in-built assumption that the state used to assess > > mergeability implies the same properties. > > Hmm. What prevents two VMAs with different vm_private_data members to > be merged? IIUC is_mergeable_vma() does not check vm_private_data. In > such a case set_vma_user_defined_fields() would override > vm_private_data of an existing VMA, no?
I think you're right that it should be a fix patch, I'll put it out of the series and send it as one. The issue here is more so vm_private_data than vm_ops. And really it's that vm_ops->mapped() wasn't called so you could have a refcount go to zero and stay at zero when it shouldn't have been, for instance. But in general though if you remove mmap_prepare and ask the same question: What prevents a merge of 2 existing VMAs that were mapped using the traditional mmap hook which somehow have entirely distinct vm_private_data and vm_ops but the same file? The answer is nothing prevents that, but there's an underlying assumption that this state is fungible for a VMA over a given range given the same file. In that case, for anything where an allocation or e.g. refcount change occurred, then vm_ops->close() will handle the decrement, and the original VMA's state should suffice. But here it's a problem because you overwrite it + don't call vm_ops->mapped()... > > > > > And if it was, it'd need fixing a different way (check the field for > > instance) > > and would apply to the legacy mmap hook also. > > > > This change is needed for the series though. > > > > > > > > > > > > > Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]> > > > > > > Reviewed-by: Suren Baghdasaryan <[email protected]> > > > > Thanks! > > > > > > > > > --- > > > > mm/vma.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > index fa784f069da4..4b74b48c29b7 100644 > > > > --- a/mm/vma.c > > > > +++ b/mm/vma.c > > > > @@ -2894,7 +2894,7 @@ static unsigned long __mmap_region(struct file > > > > *file, unsigned long addr, > > > > { > > > > struct mm_struct *mm = current->mm; > > > > struct vm_area_struct *vma = NULL; > > > > - bool have_mmap_prepare = file && file->f_op->mmap_prepare; > > > > + const bool have_mmap_prepare = file && file->f_op->mmap_prepare; > > > > VMA_ITERATOR(vmi, mm, addr); > > > > const pgoff_t anon_pgoff = addr >> PAGE_SHIFT; > > > > MMAP_STATE(map, mm, &vmi, addr, len, pgoff, anon_pgoff, > > > > vma_flags, file); > > > > @@ -2937,7 +2937,7 @@ static unsigned long __mmap_region(struct file > > > > *file, unsigned long addr, > > > > allocated_new = true; > > > > } > > > > > > > > - if (have_mmap_prepare && !map_is_anon(&map)) > > > > + if (have_mmap_prepare && allocated_new && !map_is_anon(&map)) > > > > set_vma_user_defined_fields(vma, &map); > > > > > > > > __mmap_complete(&map, vma); > > > > > > > > -- > > > > 2.55.0 > > > > > > > > -- > > Cheers, Lorenzo -- Cheers, Lorenzo
