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

[Liam Howlett <liam.howlett@oracle.com>]

* Carlos Llamas <cmllamas@google.com> [220912 15:11]:
> On Fri, Sep 09, 2022 at 01:03:08PM -0700, Suren Baghdasaryan wrote:
> > On Fri, Sep 9, 2022 at 12:35 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > Does this mean that users of async calls such as find_vma() can't rely
> > > on mmap_lock to avoid racing with remove_vma()? I see the following
> > > pattern is used quite often:
> > >
> > >         mmap_read_lock(mm);
> > >         vma = find_vma(mm, addr);
> > >         [...]
> > >         mmap_read_unlock(mm);
> > >
> > > Is this not a real concern? I'd drop the asserts from binder and call it
> > > a day. However, we would also need to fix our race with vm_ops->close().
> > 
> > I think by the time exit_mmap() calls remove_vma() there can be no
> > other user of that mm to race with, even oom-reaper would have
> > finished by then (see:
> > https://elixir.bootlin.com/linux/v5.15.67/source/mm/mmap.c#L3157).
> > So, generally remove_vma() would be done under mmap_lock write
> > protection but in case of exit_mmap() that's not necessary. Michal,
> > please correct me if I'm wrong.
> 
> I see, that makes more sense.
> 
> Then it sounds to me like binder should be using mmget_not_zero() to
> serialize against exit_mmap() during these async calls. I'll have a
> closer look at this change.
> 
> Also, we should drop the mmap_lock asserts in binder from v5.15 as the
> expectations there are incorrect. Again, this was done in [1], but for
> different reasons. We could simply amend a small note to the commit log
> with an accurate reason for the backport.
> 
> Liam, wdyt?

It sounds like the binder_alloc vma_vm_mm is being used unsafely as
well?  I'd actually go the other way with this and try to add more
validation that are optimized out on production builds.  Since binder is
saving a pointer to the mm struct and was saving the vma ponter, we
should be very careful around how we use them. Is the mutex in
binder_alloc protection enough for the vma binder buffers uses?  How is
the close() not being called before the exit_mmap() path?

When you look at the mmget_not_zero() stuff, have a look at
binder_alloc_new_buf_locked().  I think it is unsafely using the
vma_vm_mm pointer without calling mmget_not_zero(), but the calling
function is rather large so I'm not sure.


> 
> [1] https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/