* [PATCH v4] shmem: fix a race between shmem_unused_huge_shrink and shmem_evict_inode
@ 2021-11-25 3:12 Gang Li
2021-11-25 4:07 ` Muchun Song
0 siblings, 1 reply; 2+ messages in thread
From: Gang Li @ 2021-11-25 3:12 UTC (permalink / raw)
To: Hugh Dickins, Andrew Morton, Kirill A. Shutemov
Cc: stable, songmuchun, Gang Li, linux-mm, linux-kernel
This patch fixes a data race in commit 779750d20b93 ("shmem: split huge pages
beyond i_size under memory pressure").
Here are call traces causing race:
Call Trace 1:
shmem_unused_huge_shrink+0x3ae/0x410
? __list_lru_walk_one.isra.5+0x33/0x160
super_cache_scan+0x17c/0x190
shrink_slab.part.55+0x1ef/0x3f0
shrink_node+0x10e/0x330
kswapd+0x380/0x740
kthread+0xfc/0x130
? mem_cgroup_shrink_node+0x170/0x170
? kthread_create_on_node+0x70/0x70
ret_from_fork+0x1f/0x30
Call Trace 2:
shmem_evict_inode+0xd8/0x190
evict+0xbe/0x1c0
do_unlinkat+0x137/0x330
do_syscall_64+0x76/0x120
entry_SYSCALL_64_after_hwframe+0x3d/0xa2
A simple explanation:
Image there are 3 items in the local list (@list).
In the first traversal, A is not deleted from @list.
1) A->B->C
^
|
pos (leave)
In the second traversal, B is deleted from @list. Concurrently, A is
deleted from @list through shmem_evict_inode() since last reference counter of
inode is dropped by other thread. Then the @list is corrupted.
2) A->B->C
^ ^
| |
evict pos (drop)
We should make sure the inode is either on the global list or deleted from
any local list before iput().
Fixed by moving inodes back to global list before we put them.
Fixes: 779750d20b93 ("shmem: split huge pages beyond i_size under memory pressure")
Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
---
mm/shmem.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 9023103ee7d8..e6ccb2a076ff 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -569,7 +569,6 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
/* inode is about to be evicted */
if (!inode) {
list_del_init(&info->shrinklist);
- removed++;
goto next;
}
@@ -577,12 +576,12 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
if (round_up(inode->i_size, PAGE_SIZE) ==
round_up(inode->i_size, HPAGE_PMD_SIZE)) {
list_move(&info->shrinklist, &to_remove);
- removed++;
goto next;
}
list_move(&info->shrinklist, &list);
next:
+ sbinfo->shrinklist_len--;
if (!--batch)
break;
}
@@ -602,7 +601,7 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
inode = &info->vfs_inode;
if (nr_to_split && split >= nr_to_split)
- goto leave;
+ goto move_back;
page = find_get_page(inode->i_mapping,
(inode->i_size & HPAGE_PMD_MASK) >> PAGE_SHIFT);
@@ -616,38 +615,43 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
}
/*
- * Leave the inode on the list if we failed to lock
- * the page at this time.
+ * Move the inode on the list back to shrinklist if we failed
+ * to lock the page at this time.
*
* Waiting for the lock may lead to deadlock in the
* reclaim path.
*/
if (!trylock_page(page)) {
put_page(page);
- goto leave;
+ goto move_back;
}
ret = split_huge_page(page);
unlock_page(page);
put_page(page);
- /* If split failed leave the inode on the list */
+ /* If split failed move the inode on the list back to shrinklist */
if (ret)
- goto leave;
+ goto move_back;
split++;
drop:
list_del_init(&info->shrinklist);
- removed++;
-leave:
+ goto put;
+move_back:
+ /*
+ * Make sure the inode is either on the global list or deleted from
+ * any local list before iput() since it could be deleted in another
+ * thread once we put the inode (then the local list is corrupted).
+ */
+ spin_lock(&sbinfo->shrinklist_lock);
+ list_move(&info->shrinklist, &sbinfo->shrinklist);
+ sbinfo->shrinklist_len++;
+ spin_unlock(&sbinfo->shrinklist_lock);
+put:
iput(inode);
}
- spin_lock(&sbinfo->shrinklist_lock);
- list_splice_tail(&list, &sbinfo->shrinklist);
- sbinfo->shrinklist_len -= removed;
- spin_unlock(&sbinfo->shrinklist_lock);
-
return split;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v4] shmem: fix a race between shmem_unused_huge_shrink and shmem_evict_inode
2021-11-25 3:12 [PATCH v4] shmem: fix a race between shmem_unused_huge_shrink and shmem_evict_inode Gang Li
@ 2021-11-25 4:07 ` Muchun Song
0 siblings, 0 replies; 2+ messages in thread
From: Muchun Song @ 2021-11-25 4:07 UTC (permalink / raw)
To: Gang Li
Cc: Hugh Dickins, Andrew Morton, Kirill A. Shutemov, linux- stable,
Linux Memory Management List, LKML
On Thu, Nov 25, 2021 at 11:12 AM Gang Li <ligang.bdlg@bytedance.com> wrote:
>
> This patch fixes a data race in commit 779750d20b93 ("shmem: split huge pages
> beyond i_size under memory pressure").
>
> Here are call traces causing race:
>
> Call Trace 1:
> shmem_unused_huge_shrink+0x3ae/0x410
> ? __list_lru_walk_one.isra.5+0x33/0x160
> super_cache_scan+0x17c/0x190
> shrink_slab.part.55+0x1ef/0x3f0
> shrink_node+0x10e/0x330
> kswapd+0x380/0x740
> kthread+0xfc/0x130
> ? mem_cgroup_shrink_node+0x170/0x170
> ? kthread_create_on_node+0x70/0x70
> ret_from_fork+0x1f/0x30
>
> Call Trace 2:
> shmem_evict_inode+0xd8/0x190
> evict+0xbe/0x1c0
> do_unlinkat+0x137/0x330
> do_syscall_64+0x76/0x120
> entry_SYSCALL_64_after_hwframe+0x3d/0xa2
>
> A simple explanation:
>
> Image there are 3 items in the local list (@list).
> In the first traversal, A is not deleted from @list.
>
> 1) A->B->C
> ^
> |
> pos (leave)
>
> In the second traversal, B is deleted from @list. Concurrently, A is
> deleted from @list through shmem_evict_inode() since last reference counter of
> inode is dropped by other thread. Then the @list is corrupted.
>
> 2) A->B->C
> ^ ^
> | |
> evict pos (drop)
>
> We should make sure the inode is either on the global list or deleted from
> any local list before iput().
>
> Fixed by moving inodes back to global list before we put them.
>
> Fixes: 779750d20b93 ("shmem: split huge pages beyond i_size under memory pressure")
> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
You have forgotten my Reviewed-by and Kirill A. Shutemov's Acked-by
as well as Cc: stable@vger.kernel.org.
> ---
> mm/shmem.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 9023103ee7d8..e6ccb2a076ff 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -569,7 +569,6 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> /* inode is about to be evicted */
> if (!inode) {
> list_del_init(&info->shrinklist);
> - removed++;
I believe there is a warning about @removed since it's unused.
> goto next;
> }
>
> @@ -577,12 +576,12 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> if (round_up(inode->i_size, PAGE_SIZE) ==
> round_up(inode->i_size, HPAGE_PMD_SIZE)) {
> list_move(&info->shrinklist, &to_remove);
> - removed++;
> goto next;
> }
>
> list_move(&info->shrinklist, &list);
> next:
> + sbinfo->shrinklist_len--;
> if (!--batch)
> break;
> }
> @@ -602,7 +601,7 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> inode = &info->vfs_inode;
>
> if (nr_to_split && split >= nr_to_split)
> - goto leave;
> + goto move_back;
>
> page = find_get_page(inode->i_mapping,
> (inode->i_size & HPAGE_PMD_MASK) >> PAGE_SHIFT);
> @@ -616,38 +615,43 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> }
>
> /*
> - * Leave the inode on the list if we failed to lock
> - * the page at this time.
> + * Move the inode on the list back to shrinklist if we failed
> + * to lock the page at this time.
> *
> * Waiting for the lock may lead to deadlock in the
> * reclaim path.
> */
> if (!trylock_page(page)) {
> put_page(page);
> - goto leave;
> + goto move_back;
> }
>
> ret = split_huge_page(page);
> unlock_page(page);
> put_page(page);
>
> - /* If split failed leave the inode on the list */
> + /* If split failed move the inode on the list back to shrinklist */
> if (ret)
> - goto leave;
> + goto move_back;
>
> split++;
> drop:
> list_del_init(&info->shrinklist);
> - removed++;
> -leave:
> + goto put;
> +move_back:
> + /*
> + * Make sure the inode is either on the global list or deleted from
> + * any local list before iput() since it could be deleted in another
> + * thread once we put the inode (then the local list is corrupted).
> + */
> + spin_lock(&sbinfo->shrinklist_lock);
> + list_move(&info->shrinklist, &sbinfo->shrinklist);
> + sbinfo->shrinklist_len++;
> + spin_unlock(&sbinfo->shrinklist_lock);
> +put:
> iput(inode);
> }
>
> - spin_lock(&sbinfo->shrinklist_lock);
> - list_splice_tail(&list, &sbinfo->shrinklist);
> - sbinfo->shrinklist_len -= removed;
> - spin_unlock(&sbinfo->shrinklist_lock);
> -
> return split;
> }
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-25 4:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-25 3:12 [PATCH v4] shmem: fix a race between shmem_unused_huge_shrink and shmem_evict_inode Gang Li
2021-11-25 4:07 ` Muchun Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).