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

[Luigi Rizzo <lrizzo@google.com>]

On Wed, Sep 8, 2021 at 4:16 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Sep 08, 2021 at 10:53:26AM -0300, Jason Gunthorpe wrote:
> > 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.
...
> > > 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.
>
> The only right solution to all of this is the below. That function
> downright subverts all the locking rules we have. Spreading the hacks
> any further than that one function is absolutely unacceptable.

I'd be inclined to agree that we should not introduce hacks around
locking rules. I don't know enough about the constraints of
bpf/stackmap, how much of a performance penalty do we pay with Peter's
patch,
and ow often one is expected to call this function ?

cheers
luigi