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

[Suren Baghdasaryan <surenb@google.com>]

On Thu, Sep 8, 2022 at 1:28 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> Hi,
>
> I've received several reports of a mmap_assert_locked() BUG triggering
> at binder_alloc_vma_close() in the v5.15 stable tree. This makes sense
> as the following two commits were recently picked for stable:
>
> a43cfc87caaf ("android: binder: stop saving a pointer to the VMA")
> b0cab80ecd54 ("android: binder: fix lockdep check on clearing vma")
>
> In such commits mmap_asserts are added to binder_alloc_set_vma() which
> is called back from vma->vm_ops->close() and file->f_op->mmap().
>
> 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
> undoubtedly it leads to the following BUG:
>
>         kernel BUG at include/linux/mmap_lock.h:156!
>         Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>         [...]
>         Call trace:
>         binder_alloc_vma_close+0x14c/0x150
>         binder_vma_close+0xa4/0x184
>         remove_vma+0xa4/0x108
>         exit_mmap+0x320/0x424
>         __mmput+0xb4/0x2b4
>         mmput+0x8c/0xb0
>         exit_mm+0x51c/0x6c8
>         do_exit+0x488/0x1bf4
>         do_group_exit+0x118/0x270
>         [...]
>
> Note that I have removed such asserts from the binder driver here:
> https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/
> as a random driver didn't seem to me like the appropriate place to
> validate the mm stack locking. Since this patch is not going to the
> stable trees, the asserts still exist in v5.15.
>
> So, what is the proper fix for this v5.15 BUG? Should f798a1d4f94d
> be picked for v5.15 stable? Or should binder be fixed instead?

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().
However in 5.15 kernel that assumption is incorrect. In that case IMHO
the backport needs to drop that invalid expectation.
Thanks,
Suren.

>
> Thanks,
> --
> Carlos Llamas