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
