Re: [PATCH mm/bpf v2] mm: bpf: add find_vma_no_check() without lockdep_assert on mm->mmap_lock

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Sep 08, 2021 at 02:20:17PM +0200, Daniel Borkmann wrote:

> > The warning is due to commit 5b78ed24e8ec("mm/pagemap: add mmap_assert_locked() annotations to find_vma*()")
> > which added mmap_assert_locked() in find_vma() function. The mmap_assert_locked() function
> > asserts that mm->mmap_lock needs to be held. But this is not the case for
> > bpf_get_stack() or bpf_get_stackid() helper (kernel/bpf/stackmap.c), which
> > uses mmap_read_trylock_non_owner() instead. Since mm->mmap_lock is not held
> > in bpf_get_stack[id]() use case, the above warning is emitted during test run.
> > 
> > This patch added function find_vma_no_check() which does not have mmap_assert_locked() call and
> > bpf_get_stack[id]() helpers call find_vma_no_check() instead. This resolved the above warning.
> > 
> > I didn't use __find_vma() name because it has been used in drivers/gpu/drm/i915/i915_gpu_error.c:
> >      static struct i915_vma_coredump *
> >      __find_vma(struct i915_vma_coredump *vma, const char *name) { ... }
> > 
> > Cc: Luigi Rizzo <lrizzo@google.com>
> > Fixes: 5b78ed24e8ec("mm/pagemap: add mmap_assert_locked() annotations to find_vma*()")
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> 
> Luigi / Liam / Andrew, if the below looks reasonable to you, any objections to route the
> fix with your ACKs via bpf tree to Linus (or strong preference via -mm fixes)?

Michel added this remark along with the mmap_read_trylock_non_owner:

    It's still not ideal that bpf/stackmap subverts the lock ownership in this
    way.  Thanks to Peter Zijlstra for suggesting this API as the least-ugly
    way of addressing this in the short term.

Subverting lockdep and then adding more and more core MM APIs to
support this seems quite a bit more ugly than originally expected.

Michel's original idea to split out the lockdep abuse and put it only
in BPF is probably better. Obtain the mmap_read_trylock normally as
owner and then release ownership only before triggering the work. At
least lockdep will continue to work properly for the find_vma.

Jason