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

[Kemeng Shi <shikemeng@huaweicloud.com>]

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!

> 
> 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);
>>   
>