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@xxxxxxxxxx>
> > Fixes: 5b78ed24e8ec("mm/pagemap: add mmap_assert_locked() annotations to find_vma*()")
> > Signed-off-by: Yonghong Song <yhs@xxxxxx>
>
> 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
