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 06:30:52PM +0000, Liam Howlett wrote:

>  /* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */
> -struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> +struct vm_area_struct *find_vma_non_owner(struct mm_struct *mm,
> +					 unsigned long addr)
>  {
>  	struct rb_node *rb_node;
>  	struct vm_area_struct *vma;
>  
> -	mmap_assert_locked(mm);
> +	VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
>  	/* Check the cache first. */
>  	vma = vmacache_find(mm, addr);
>  	if (likely(vma))
> @@ -2325,6 +2326,11 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  	return vma;
>  }
>  
> +struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> +{
> +	lockdep_assert_held(&mm->mmap_lock);
> +	return find_vma_non_owner(mm, addr);
> +}
>  EXPORT_SYMBOL(find_vma);
>  
>  /*
> 
> 
> Although this leaks more into the mm API and was referred to as ugly
> previously, it does provide a working solution and still maintains the
> same level of checking.

I think it is no better than before.

The solution must be to not break lockdep in the BPF side. If Peter's
reworked algorithm is not OK then BPF should drop/acquire the lockdep
when it punts the unlock to the WQ.

'non-owner' is just a nice way of saying 'the caller is messing with
lockdep', it is not a sane way to design APIs

Jason