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

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Sep 9, 2022 at 12:35 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Thu, Sep 08, 2022 at 03:33:52PM -0700, Suren Baghdasaryan wrote:
> > > However, mmap_locking was only added to the exit_mmap() path in commit
> > > f798a1d4f94d ("mm: fix use-after-free bug when mm->mmap is reused after
> > > being freed") and since this patch doesn't exist in v5.15 stable tree
>
> Sorry, I meant 64591e8605d6 ("mm: protect free_pgtables with mmap_lock
> write lock in exit_mmap") here, that's when protection was added.
>
> > IIUC, the binder patches are backported to 5.15 kernel and they expect
> > mmap_lock to be held during remove_vma() operation in exit_mmap().
>
> Correct!
>
> > However in 5.15 kernel that assumption is incorrect. In that case IMHO
> > the backport needs to drop that invalid expectation.
>
> 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.