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

[sashiko-bot@kernel.org]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-fixes_shmem-v3-0-4ce3baf2db4b@kernel.org?part=6