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

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/1/30 0:17, Liam R. Howlett wrote:
> * 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)?

The fork path holds the mmap lock from parent A and other childs(except first child B) while hugetlbfs path
holds the mmap lock from first child B. So the mmap lock won't help here because it comes from different mm.
Or am I miss something?

Thanks.

> 
> 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
> 
> 
> .
>