Re: [PATCH v2 1/5] mm: shmem: avoid unpaired folio_unlock() in shmem_swapin_folio()

Re: [PATCH v2 1/5] mm: shmem: avoid unpaired folio_unlock() in shmem_swapin_folio()

[Kairui Song]

On Thu, May 15, 2025 at 2:54 PM Kemeng Shi <shikemeng@huaweicloud.com> wrote:
>
> If we get a folio from swap_cache_get_folio() successfully but encounter
> a failure before the folio is locked, we will unlock the folio which was
> not previously locked.
> Put the folio and set it to NULL when a failure occurs before the folio
> is locked to fix the issue.
>
> Fixes: 058313515d5aa ("mm: shmem: fix potential data corruption during shmem swapin")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/shmem.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 99327c30507c..980fa15f393e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2335,6 +2335,8 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>                  */
>                 split_order = shmem_split_large_entry(inode, index, swap, gfp);
>                 if (split_order < 0) {
> +                       folio_put(folio);
> +                       folio = NULL;
>                         error = split_order;
>                         goto failed;
>                 }

Nice fix, I also noticed this and included the same fix in the swap
table series and forgot to split that out. We should merge this clean
fix first:

Reviewed-by: Kairui Song <kasong@tencent.com>

[PATCH v2 0/5] Some random fixes and cleanup to shmem

[Kemeng Shi]

v1->v2:
-Collect RVB and some minor improvements

This series contains some simple fixes and cleanup which are made during
learning shmem. More details can be found in respective patches. Thanks.

Kemeng Shi (5):
  mm: shmem: avoid unpaired folio_unlock() in shmem_swapin_folio()
  mm: shmem: add missing shmem_unacct_size() in __shmem_file_setup()
  mm/shmem: Fix potential dead loop in shmem_unuse()
  mm: shmem: only remove inode from swaplist when it's swapped page
    count is 0
  mm/shmem: remove unneeded xa_is_value() check in
    shmem_unuse_swap_entries()

 mm/shmem.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

-- 
2.30.0

[PATCH v2 1/5] mm: shmem: avoid unpaired folio_unlock() in shmem_swapin_folio()

[Kemeng Shi]

If we get a folio from swap_cache_get_folio() successfully but encounter
a failure before the folio is locked, we will unlock the folio which was
not previously locked.
Put the folio and set it to NULL when a failure occurs before the folio
is locked to fix the issue.

Fixes: 058313515d5aa ("mm: shmem: fix potential data corruption during shmem swapin")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/shmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 99327c30507c..980fa15f393e 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2335,6 +2335,8 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
 		 */
 		split_order = shmem_split_large_entry(inode, index, swap, gfp);
 		if (split_order < 0) {
+			folio_put(folio);
+			folio = NULL;
 			error = split_order;
 			goto failed;
 		}
-- 
2.30.0

[PATCH v2 2/5] mm: shmem: add missing shmem_unacct_size() in __shmem_file_setup()

[Kemeng Shi]

We will miss shmem_unacct_size() when is_idmapped_mnt() returns a failure.
Move is_idmapped_mnt() before shmem_acct_size() to fix the issue.

Fixes: 7a80e5b8c6fa7 ("shmem: support idmapped mounts for tmpfs")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/shmem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 980fa15f393e..495e661eb8bb 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -5812,12 +5812,12 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name,
 	if (size < 0 || size > MAX_LFS_FILESIZE)
 		return ERR_PTR(-EINVAL);
 
-	if (shmem_acct_size(flags, size))
-		return ERR_PTR(-ENOMEM);
-
 	if (is_idmapped_mnt(mnt))
 		return ERR_PTR(-EINVAL);
 
+	if (shmem_acct_size(flags, size))
+		return ERR_PTR(-ENOMEM);
+
 	inode = shmem_get_inode(&nop_mnt_idmap, mnt->mnt_sb, NULL,
 				S_IFREG | S_IRWXUGO, 0, flags);
 	if (IS_ERR(inode)) {
-- 
2.30.0

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

[Kemeng Shi]

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 495e661eb8bb..aeeddf612baa 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);
@@ -1523,13 +1524,15 @@ 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 (atomic_dec_and_test(&info->stop_eviction))
 			wake_up_var(&info->stop_eviction);
 		if (error)
 			break;
+		if (list_empty(&info->swaplist))
+			goto start_over;
+		next = list_next_entry(info, swaplist);
+		if (!info->swapped)
+			list_del_init(&info->swaplist);
 	}
 	mutex_unlock(&shmem_swaplist_mutex);
 
-- 
2.30.0

[PATCH v2 4/5] mm: shmem: only remove inode from swaplist when it's swapped page count is 0

[Kemeng Shi]

Even if we fail to allocate a swap entry, the inode might have previously
allocated entry and we might take inode containing swap entry off swaplist.
As a result, try_to_unuse() may enter a potential dead loop to repeatedly
look for inode and clean it's swap entry.
Only take inode off swaplist when it's swapped page count is 0 to fix the
issue.

Fixes: b487a2da3575b ("mm, swap: simplify folio swap allocation")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/shmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index aeeddf612baa..07b8e1400c67 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1651,8 +1651,8 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 		BUG_ON(folio_mapped(folio));
 		return swap_writepage(&folio->page, wbc);
 	}
-
-	list_del_init(&info->swaplist);
+	if (!info->swapped)
+		list_del_init(&info->swaplist);
 	mutex_unlock(&shmem_swaplist_mutex);
 	if (nr_pages > 1)
 		goto try_split;
-- 
2.30.0

[PATCH v2 5/5] mm/shmem: remove unneeded xa_is_value() check in shmem_unuse_swap_entries()

[Kemeng Shi]

As only value entry will be added to fbatch in shmem_find_swap_entries(),
there is no need to do xa_is_value() check in shmem_unuse_swap_entries().

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/shmem.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 07b8e1400c67..4b42419ce6b2 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1446,8 +1446,6 @@ static int shmem_unuse_swap_entries(struct inode *inode,
 	for (i = 0; i < folio_batch_count(fbatch); i++) {
 		struct folio *folio = fbatch->folios[i];
 
-		if (!xa_is_value(folio))
-			continue;
 		error = shmem_swapin_folio(inode, indices[i], &folio, SGP_CACHE,
 					mapping_gfp_mask(mapping), NULL, NULL);
 		if (error == 0) {
-- 
2.30.0

Re: [PATCH v2 4/5] mm: shmem: only remove inode from swaplist when it's swapped page count is 0

[Baolin Wang]

On 2025/5/15 23:47, Kemeng Shi wrote:
> Even if we fail to allocate a swap entry, the inode might have previously
> allocated entry and we might take inode containing swap entry off swaplist.
> As a result, try_to_unuse() may enter a potential dead loop to repeatedly
> look for inode and clean it's swap entry.
> Only take inode off swaplist when it's swapped page count is 0 to fix the
> issue.
> 
> Fixes: b487a2da3575b ("mm, swap: simplify folio swap allocation")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/shmem.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index aeeddf612baa..07b8e1400c67 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1651,8 +1651,8 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>   		BUG_ON(folio_mapped(folio));
>   		return swap_writepage(&folio->page, wbc);
>   	}
> -
> -	list_del_init(&info->swaplist);
> +	if (!info->swapped)
> +		list_del_init(&info->swaplist);
>   	mutex_unlock(&shmem_swaplist_mutex);
>   	if (nr_pages > 1)
>   		goto try_split;

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

[Baolin Wang]

On 2025/5/15 23:47, 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>

LGTM. Thanks.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/shmem.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 495e661eb8bb..aeeddf612baa 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);
> @@ -1523,13 +1524,15 @@ 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 (atomic_dec_and_test(&info->stop_eviction))
>   			wake_up_var(&info->stop_eviction);
>   		if (error)
>   			break;
> +		if (list_empty(&info->swaplist))
> +			goto start_over;
> +		next = list_next_entry(info, swaplist);
> +		if (!info->swapped)
> +			list_del_init(&info->swaplist);
>   	}
>   	mutex_unlock(&shmem_swaplist_mutex);
>   

Re: [PATCH v2 4/5] mm: shmem: only remove inode from swaplist when it's swapped page count is 0

[Kairui Song]

On Thu, May 15, 2025 at 2:54 PM Kemeng Shi <shikemeng@huaweicloud.com> wrote:
>
> Even if we fail to allocate a swap entry, the inode might have previously
> allocated entry and we might take inode containing swap entry off swaplist.
> As a result, try_to_unuse() may enter a potential dead loop to repeatedly
> look for inode and clean it's swap entry.
> Only take inode off swaplist when it's swapped page count is 0 to fix the
> issue.
>
> Fixes: b487a2da3575b ("mm, swap: simplify folio swap allocation")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  mm/shmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index aeeddf612baa..07b8e1400c67 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1651,8 +1651,8 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>                 BUG_ON(folio_mapped(folio));
>                 return swap_writepage(&folio->page, wbc);
>         }
> -
> -       list_del_init(&info->swaplist);
> +       if (!info->swapped)
> +               list_del_init(&info->swaplist);
>         mutex_unlock(&shmem_swaplist_mutex);
>         if (nr_pages > 1)
>                 goto try_split;
> --
> 2.30.0
>
>

Thanks for the fix!

Reviewed-by: Kairui Song <kasong@tencent.com>