Re: BUG in binder_vma_close() at mmap_assert_locked() in stable v5.15

[Carlos Llamas <cmllamas@google.com>]

On Wed, Sep 28, 2022 at 04:21:27PM -0700, Suren Baghdasaryan wrote:
> > However, I wonder why isn't ->mmap() being undone for this exit path in
> > mmap_region()? If anything fails _after_ call_mmap() it seems we
> > silently unmap and free the vma. What about allocations, atomic_incs,
> > and anything done inside ->mmap()?
> 
> I think if ->mmap() fails it is expected to undo its own changes
> before returning the error. mmap_region() has no idea what kind of
> changes ->mmap() has done before it failed, therefore it can't undo
> them. At least that's how I understand it.

I agree ->mmap() should undo its own changes if it fails. However, the
scenario I'm referring to is an error _after_ ->mmap() call succeeds.
In this case it can be the arch_validate_flags() check. On error, the
vma is torn down and the drivers are never informed about it.

> > Shouldn't a ->close() be needed here
> > to undo these things as such:
> > --
> > @@ -1872,6 +1889,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >
> >         return addr;
> >
> > +undo_mmap:
> > +       if (vma->vm_ops && vma->vm_ops->close)
> > +               vma->vm_ops->close(vma);
> >  unmap_and_free_vma:
> >         fput(vma->vm_file);
> >         vma->vm_file = NULL;
> > --
> >
> 
> I don't see mmap_region() calling vma->vm_ops->open() anywhere. So why
> would it have to call vma->vm_ops->close()?

My understanding is ->mmap() is called on the very first mapping and
vm_ops->close() is subsequently called whenever a mapping is removed.
So IIUC a vm_ops->open() is not required in this exchange.

There are multiple places where I can see rely on this behavior. Just to
provide an example, look at coda_file_mmap() which allocates cvm_ops and
increments references on ->mmap(). It then expects coda_vm_close() to be
called when the vma is removed to decrement these references and lastly
free cvm_ops. However, if mmap_region() fails after ->mmap() call then
vm_ops->close() is never invoked and the reference count in coda is now
off making cvm_ops leaked memory.

Other type of issues happen depending on the ->mmap() implementation. In
our case it was a BUG_ON() in binder.

I don't know if adding vm_ops->close() to the exit error path is an
appropriate solution. Perhaps ->mmap() should be a point of no return
instead and vm_flags should be validated earlier? I would be concerned
about drivers modifying the vm_flags during ->mmap() though. I'll send
out a patch to get more feedback on this vm_ops->close().

--
Carlos Llamas