traversing vma on nommu

[Liam R. Howlett]

* Hajime Tazaki <thehajime at gmail.com> [241108 08:08]:
> 
> 
> On Fri, 08 Nov 2024 09:39:05 +0900,
> Hajime Tazaki wrote:
> 
> > > > after adding mt_validate() across various places, I found
> > > > vma_iter_clear() in nommu.c triggers this issue on nommu UML.
> > > > 
> > > > I'm not totally understanding but if I changed the part with
> > > > vma_iter_clear_gfp(), the issue (and validation error reports) are
> > > > disappeared.
> > > > 
> > > > diff --git a/mm/nommu.c b/mm/nommu.c
> > > > index 385b0c15add8..b5c11bbd69de 100644
> > > > --- a/mm/nommu.c
> > > > +++ b/mm/nommu.c
> > > > @@ -581,7 +581,8 @@ static int delete_vma_from_mm(struct vm_area_struct *vma)
> > > >  	cleanup_vma_from_mm(vma);
> > > >  
> > > >  	/* remove from the MM's tree and list */
> > > > -	vma_iter_clear(&vmi);
> > > > +	vma_iter_clear_gfp(&vmi, vma->vm_start, vma->vm_end, GFP_KERNEL);
> > > > +	mt_validate(&current->mm->mm_mt);
> > > >  	return 0;
> > > >  }
> > > > 
> > > > do you think this is an appropriate fix ?
> > > 
> > > No.  Something is going wrong in regards to the vma iterator setup.  If
> > > the values are incorrect on the preallocation then you may not have
> > > enough memory to do the store.  You may end up in a situation where the
> > > vma remains in the tree but the vm_file is removed from the interval
> > > tree and that doesn't seem like a good idea.
> 
> sorry for asking many times.

First, don't be sorry.  This is very helpful and you are doing
everything correct.  I appreciate the help and the politeness of
debugging an issue that is probably very frustrating for you.

> 
> after another random attempt trying to avoid the issue, the patch
> below also fixed it.  sequential nulls are also gone.
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 385b0c15add8..0c708f85408d 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -573,7 +573,7 @@ static int delete_vma_from_mm(struct vm_area_struct *vma)
>         VMA_ITERATOR(vmi, vma->vm_mm, vma->vm_start);
>  
>         vma_iter_config(&vmi, vma->vm_start, vma->vm_end);
> -       if (vma_iter_prealloc(&vmi, vma)) {
> +       if (vma_iter_prealloc(&vmi, NULL)) {

Yes, this is the correct fix and this is a bug.  It should be backported
with the correct tags (Cc: Stable, Fixes), send it to Andrew Morton and
cc me and the mm list.

The preallocation calculation is dependent on the argument passed and we
are passing the wrong argument.

Reviewed-by: Liam R. Howlett <Liam.Howlett at Oracle.com>

>                 pr_warn("Allocation of vma tree for process %d failed\n",
>                        current->pid);
>                 return -ENOMEM;
> 
> if this is a right fix, the following commit introduced this issue
> while restructuring the interface.
> 
> commit b5df09226450165c434084d346fcb6d4858b0d52
> Author: Liam R. Howlett <Liam.Howlett at oracle.com>
> Date:   Mon Jul 24 14:31:52 2023 -0400
> 
>     mm: set up vma iterator for vma_iter_prealloc() calls
> 

That sounds about right for the Fixes tag.

Please Cc me on your UM Linux patches for nommu as it may make my
testing life easier as well, if it's not too much trouble.

Thanks for finding the issue,
Liam