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

[Carlos Llamas <cmllamas@google.com>]

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?

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