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

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

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.

IIUC, the problem is the check for list_empty(), and that can race.

So we must rely entirely on stop_eviction under lock.

diff --git a/mm/shmem.c b/mm/shmem.c
index c44d2dd71c89..e518fb8e3350 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1643,17 +1643,18 @@ static void shmem_evict_inode(struct inode *inode)
                 * Writeout may still use the inode after swapoff has removed it
                 * from the swaplist, so always wait at least once.
                 */
-               do {
+               while (true) {
                        wait_var_event(&info->stop_eviction,
                                       !atomic_read(&info->stop_eviction));
-                       if (list_empty(&info->swaplist))
-                               break;
+                       /* Verify under lock to sync with concurrent swapoff. */
                        spin_lock(&shmem_swaplist_lock);
-                       /* ...but beware of the race if we peeked too early */
-                       if (!atomic_read(&info->stop_eviction))
+                       if (!atomic_read(&info->stop_eviction)) {
                                list_del_init(&info->swaplist);
+                               spin_unlock(&shmem_swaplist_lock);
+                               break;
+                       }
                        spin_unlock(&shmem_swaplist_lock);
-               } while (!list_empty(&info->swaplist));
+               }
        }

        simple_xattrs_free(&sbinfo->xa_cache, &info->xattrs, sbinfo->max_inodes ? &freed : NULL);


But now we always have to grab the lock at least once.

Do we care? My intuition says "no". Thoughts? Alternatives?

-- 
Cheers,

David