Re: [PATCH v3 06/15] mm/shmem: hold off eviction in shmem_writeout()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/28/26 21:51, David Hildenbrand (Arm) wrote:
> On 10/28/26 21:35, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] mm/shmem: use-after-free in shmem_evict_inode() on concurrent swapoff
>> --
>>
>> commit bfe337ff80d886d314a9991abae8133932277cd0
>> Author: Josef Bacik <josef@toxicpanda.com>
>>
>> mm/shmem: hold off eviction in shmem_writeout()
>>
>> This commit prevents inode eviction during shmem_writeout() by incrementing
>> info->stop_eviction before removing the folio from the page cache and
>> decrementing it after swap_writeout() and any potential page cache
>> reinsertion complete. It also updates shmem_evict_inode() to always wait at
>> least once for info->stop_eviction to reach zero even if the inode was
>> already removed from the swaplist.
>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index e3e4bbbe1d802..eddd5870be072 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -1616,6 +1616,17 @@ static int shmem_setattr(const struct mnt_idmap *idmap,
>>>  	return error;
>>>  }
>>>  
>>> +static void shmem_disallow_eviction(struct shmem_inode_info *info)
>>> +{
>>> +	atomic_inc(&info->stop_eviction);
>>> +}
>>> +
>>> +static void shmem_allow_eviction(struct shmem_inode_info *info)
>>> +{
>>> +	if (atomic_dec_and_test(&info->stop_eviction))
>>> +		wake_up_var(&info->stop_eviction);
>>> +}
>>> +
>>>  static void shmem_evict_inode(struct inode *inode)
>>>  {
>>>  	struct shmem_inode_info *info = SHMEM_I(inode);
>>> @@ -1628,16 +1639,21 @@ static void shmem_evict_inode(struct inode *inode)
>>>  		mapping_set_exiting(inode->i_mapping);
>>>  		shmem_truncate_range(inode, 0, (loff_t)-1);
>>>  		shmem_unused_huge_del(inode);
>>> -		while (!list_empty(&info->swaplist)) {
>>> -			/* Wait while shmem_unuse() is scanning this inode... */
>>> +		/*
>>> +		 * Writeout may still use the inode after swapoff has removed it
>>> +		 * from the swaplist, so always wait at least once.
>>> +		 */
>>> +		do {
>>>  			wait_var_event(&info->stop_eviction,
>>>  				       !atomic_read(&info->stop_eviction));
>>> +			if (list_empty(&info->swaplist))
>>> +				break;
>>>  			spin_lock(&shmem_swaplist_lock);
>>>  			/* ...but beware of the race if we peeked too early */
>>>  			if (!atomic_read(&info->stop_eviction))
>>>  				list_del_init(&info->swaplist);
>>>  			spin_unlock(&shmem_swaplist_lock);
>>> -		}
>>> +		} while (!list_empty(&info->swaplist));
>>
>> [Severity: High]
>> Can this loop exit in shmem_evict_inode() while a concurrent shmem_unuse()
>> still holds info->stop_eviction > 0, leading to a use-after-free of info and
>> its embedded vfs_inode?
>>
>> Specifically, two swapoff() syscalls on different swap types (typeA and
>> typeB) can run try_to_unuse() -> shmem_unuse() concurrently while
>> shmem_evict_inode() is evicting an inode:
>>
>> 1. In shmem_evict_inode(), shmem_truncate_range() -> shmem_undo_range() ->
>>    shmem_recalc_inode() frees the swap entries and sets info->swapped = 0
>>    under info->lock (without shmem_swaplist_lock).
>>
>> 2. Just before info->swapped becomes 0, shmem_unuse(typeA) on another CPU
>>    holds shmem_swaplist_lock, sees info->swapped != 0, and is about to call
>>    shmem_disallow_eviction(info):
>>
>>    mm/shmem.c:shmem_unuse() {
>>        ...
>>        list_for_each_entry_safe(info, next, &shmem_swaplist, swaplist) {
>>            if (!info->swapped) {
>>                list_del_init(&info->swaplist);
>>                continue;
>>            }
>>            shmem_disallow_eviction(info);
>>            spin_unlock(&shmem_swaplist_lock);
>>
>>            error = shmem_unuse_inode(&info->vfs_inode, type);
>>        ...
>>    }
>>
>> 3. Before shmem_unuse(typeA) increments info->stop_eviction,
>>    shmem_evict_inode() enters the do-while loop and evaluates
>>    wait_var_event(&info->stop_eviction, !atomic_read(&info->stop_eviction)),
>>    which returns immediately because info->stop_eviction is still 0 (the
>>    "peeked too early" race noted in the comment).
>>
>> 4. shmem_unuse(typeA) then calls shmem_disallow_eviction(info) (setting
>>    info->stop_eviction to 1), drops shmem_swaplist_lock, and begins scanning
>>    &info->vfs_inode in shmem_unuse_inode().
>>
>> 5. A second concurrent swapoff running shmem_unuse(typeB) acquires
>>    shmem_swaplist_lock, sees !info->swapped is now true, and removes the
>>    inode from the list via list_del_init(&info->swaplist) without checking
>>    info->stop_eviction.
>>
>> 6. Back in shmem_evict_inode(), list_empty(&info->swaplist) is now true while
>>    info->stop_eviction is 1. Either the new unlocked
>>    if (list_empty(&info->swaplist)) break; exits the loop immediately, or
>>    shmem_evict_inode() takes shmem_swaplist_lock, skips list_del_init()
>>    because atomic_read(&info->stop_eviction) is non-zero, and then exits the
>>    loop because the while (!list_empty(&info->swaplist)) condition is false.
>>
>> Could shmem_evict_inode() then return and allow the inode to be freed while
>> shmem_unuse(typeA) is still accessing &info->vfs_inode in shmem_unuse_inode()
>> and later calling shmem_allow_eviction(info)?
>>
> 
> Two concurrent swapoffs, what at time to be alive.

Okay, I just realize that this is a preexisting issue. So I will keep this patch
as is.

We'll fix this separately later.

-- 
Cheers,

David