Re: [PATCH 3/5] mm/shmem: Fix potential dead loop in shmem_unuse()

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/5/15 09:05, Kemeng Shi wrote:
> 
> 
> on 5/14/2025 5:24 PM, Baolin Wang wrote:
>>
>>
>> On 2025/5/15 00:50, Kemeng Shi wrote:
>>> If multi shmem_unuse() for different swap type is called concurrently,
>>> a dead loop could occur as following:
>>> shmem_unuse(typeA)               shmem_unuse(typeB)
>>>    mutex_lock(&shmem_swaplist_mutex)
>>>    list_for_each_entry_safe(info, next, ...)
>>>     ...
>>>     mutex_unlock(&shmem_swaplist_mutex)
>>>     /* info->swapped may drop to 0 */
>>>     shmem_unuse_inode(&info->vfs_inode, type)
>>>
>>>                                     mutex_lock(&shmem_swaplist_mutex)
>>>                                     list_for_each_entry(info, next, ...)
>>>                                      if (!info->swapped)
>>>                                       list_del_init(&info->swaplist)
>>>
>>>                                     ...
>>>                                     mutex_unlock(&shmem_swaplist_mutex)
>>>
>>>     mutex_lock(&shmem_swaplist_mutex)
>>>     /* iterate with offlist entry and encounter a dead loop */
>>>     next = list_next_entry(info, swaplist);
>>>     ...
>>>
>>> Restart the iteration if the inode is already off shmem_swaplist list
>>> to fix the issue.
>>>
>>> Fixes: b56a2d8af9147 ("mm: rid swapoff of quadratic complexity")
>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ---
>>>    mm/shmem.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 495e661eb8bb..0fed94c2bc09 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -1505,6 +1505,7 @@ int shmem_unuse(unsigned int type)
>>>            return 0;
>>>          mutex_lock(&shmem_swaplist_mutex);
>>> +start_over:
>>>        list_for_each_entry_safe(info, next, &shmem_swaplist, swaplist) {
>>>            if (!info->swapped) {
>>>                list_del_init(&info->swaplist);
>>> @@ -1530,6 +1531,8 @@ int shmem_unuse(unsigned int type)
>>
>>          next = list_next_entry(info, swaplist);
>>          if (!info->swapped)
>>              list_del_init(&info->swaplist);
>>          if (atomic_dec_and_test(&info->stop_eviction))
>>              wake_up_var(&info->stop_eviction);
>>
>> We may still hit the list warning when calling list_del_init() for the off-list info->swaplist? So I hope we can add a check for the possible off-list:
> Hello,
> When entry is taken off list, it will be initialized to a valid empty entry
> with INIT_LIST_HEAD(). So it should be fine to call list_del_init() for
> off-list entry.
> Please correct me if I miss anything. Thanks!

Ah, yes. I got confused with list_del(), but I still think we should not 
continue to operate on an off-list entry.

>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 99327c30507c..f5ae5e2d6fb4 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1523,9 +1523,11 @@ int shmem_unuse(unsigned int type)
>>                  cond_resched();
>>
>>                  mutex_lock(&shmem_swaplist_mutex);
>> -               next = list_next_entry(info, swaplist);
>> -               if (!info->swapped)
>> -                       list_del_init(&info->swaplist);
>> +               if (!list_empty(&info->swaplist)) {
>> +                       next = list_next_entry(info, swaplist);
>> +                       if (!info->swapped)
>> +                               list_del_init(&info->swaplist);
>> +               }
>>                  if (atomic_dec_and_test(&info->stop_eviction))
>>                          wake_up_var(&info->stop_eviction);
>>                  if (error)
>>
>>>                wake_up_var(&info->stop_eviction);
>>>            if (error)
>>>                break;
>>> +        if (list_empty(&info->swaplist))
>>> +            goto start_over;
>>>        }
>>>        mutex_unlock(&shmem_swaplist_mutex);
>>>    
>>