Re: hugetlbfs: WARNING: bad unlock balance detected during MADV_REMOVE

["Liam R. Howlett" <Liam.Howlett@Oracle.com>]

* Miaohe Lin <linmiaohe@huawei.com> [240129 07:56]:
> On 2024/1/27 18:13, Miaohe Lin wrote:
> > On 2024/1/26 15:50, Muchun Song wrote:
> >>
> >>
> >>> On Jan 26, 2024, at 04:28, Thorvald Natvig <thorvald@google.com> wrote:
> >>>
> >>> We've found what appears to be a lock issue that results in a blocked
> >>> process somewhere in hugetlbfs for shared maps; seemingly from an
> >>> interaction between hugetlb_vm_op_open and hugetlb_vmdelete_list.
> >>>
> >>> Based on some added pr_warn, we believe the following is happening:
> >>> When hugetlb_vmdelete_list is entered from the child process,
> >>> vma->vm_private_data is NULL, and hence hugetlb_vma_trylock_write does
> >>> not lock, since neither __vma_shareable_lock nor __vma_private_lock
> >>> are true.
> >>>
> >>> While hugetlb_vmdelete_list is executing, the parent process does
> >>> fork(), which ends up in hugetlb_vm_op_open, which in turn allocates a
> >>> lock for the same vma.
> >>>
> >>> Thus, when the hugetlb_vmdelete_list in the child reaches the end of
> >>> the function, vma->vm_private_data is now populated, and hence
> >>> hugetlb_vma_unlock_write tries to unlock the vma_lock, which it does
> >>> not hold.
> >>
> >> Thanks for your report. ->vm_private_data was introduced since the
> >> series [1]. So I suspect it was caused by this. But I haven't reviewed
> >> that at that time (actually, it is a little complex in pmd sharing
> >> case). I saw Miaohe had reviewed many of those.
> >>
> >> CC Miaohe, maybe he has some ideas on this.
> >>
> >> [1] https://lore.kernel.org/all/20220914221810.95771-7-mike.kravetz@oracle.com/T/#m2141e4bc30401a8ce490b1965b9bad74e7f791ff
> >>
> >> Thanks.
> >>
> >>>
> >>> dmesg:
> >>> WARNING: bad unlock balance detected!
> >>> 6.8.0-rc1+ #24 Not tainted
> >>> -------------------------------------
> >>> lock/2613 is trying to release lock (&vma_lock->rw_sema) at:
> >>> [<ffffffffa94c6128>] hugetlb_vma_unlock_write+0x48/0x60
> >>> but there are no more locks to release!
> > 
> > Thanks for your report. It seems there's a race:
> > 
> >  CPU 1											CPU 2
> >  fork											hugetlbfs_fallocate
> >   dup_mmap										 hugetlbfs_punch_hole
> >    i_mmap_lock_write(mapping);								
> >    vma_interval_tree_insert_after -- Child vma is visible through i_mmap tree.
> >    i_mmap_unlock_write(mapping);
> >    hugetlb_dup_vma_private -- Clear vma_lock outside i_mmap_rwsem!			 i_mmap_lock_write(mapping);
> >    											 hugetlb_vmdelete_list
> > 											  vma_interval_tree_foreach
> > 											   hugetlb_vma_trylock_write -- Vma_lock is cleared.
> >    tmp->vm_ops->open -- Alloc new vma_lock outside i_mmap_rwsem!
> > 											   hugetlb_vma_unlock_write -- Vma_lock is assigned!!!
> > 											 i_mmap_unlock_write(mapping);
> > 
> > hugetlb_dup_vma_private and hugetlb_vm_op_open are called outside i_mmap_rwsem lock. So there will be another bugs behind it.
> > But I'm not really sure. I will take a more closed look at next week.
> 
> 
> This can be fixed by deferring vma_interval_tree_insert_after() until vma is fully initialized.
> But I'm not sure whether there're side effects with this patch.
> 
> linux-UJMmTI:/home/linmiaohe/mm # git diff
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 47ff3b35352e..2ef2711452e0 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -712,21 +712,6 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>                 } else if (anon_vma_fork(tmp, mpnt))
>                         goto fail_nomem_anon_vma_fork;
>                 vm_flags_clear(tmp, VM_LOCKED_MASK);
> -               file = tmp->vm_file;
> -               if (file) {
> -                       struct address_space *mapping = file->f_mapping;
> -
> -                       get_file(file);
> -                       i_mmap_lock_write(mapping);
> -                       if (vma_is_shared_maywrite(tmp))
> -                               mapping_allow_writable(mapping);
> -                       flush_dcache_mmap_lock(mapping);
> -                       /* insert tmp into the share list, just after mpnt */
> -                       vma_interval_tree_insert_after(tmp, mpnt,
> -                                       &mapping->i_mmap);
> -                       flush_dcache_mmap_unlock(mapping);
> -                       i_mmap_unlock_write(mapping);
> -               }
> 
>                 /*
>                  * Copy/update hugetlb private vma information.
> @@ -747,6 +732,22 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>                 if (tmp->vm_ops && tmp->vm_ops->open)
>                         tmp->vm_ops->open(tmp);
> 
> +               file = tmp->vm_file;
> +               if (file) {
> +                       struct address_space *mapping = file->f_mapping;
> +
> +                       get_file(file);
> +                       i_mmap_lock_write(mapping);
> +                       if (vma_is_shared_maywrite(tmp))
> +                               mapping_allow_writable(mapping);
> +                       flush_dcache_mmap_lock(mapping);
> +                       /* insert tmp into the share list, just after mpnt. */
> +                       vma_interval_tree_insert_after(tmp, mpnt,
> +                                       &mapping->i_mmap);
> +                       flush_dcache_mmap_unlock(mapping);
> +                       i_mmap_unlock_write(mapping);
> +               }
> +
>                 if (retval) {
>                         mpnt = vma_next(&vmi);
>                         goto loop_out;
> 
> 

How is this possible?  I thought, as specified in mm/rmap.c, that the
hugetlbfs path would be holding the mmap lock (which is also held in the
fork path)?

That is, the mmap_lock must be held before the i_mmap_lock_write()

Am I missing something?  Do we need an update to mm/rmap.c?

Thanks,
Liam