On Mon, Nov 22, 2021 at 2:41 PM Gang Li <ligang.bdlg@xxxxxxxxxxxxx> wrote:
>
> This patch fixes commit 779750d20b93 ("shmem: split huge pages
> beyond i_size under memory pressure").
>
> iput out of sbinfo->shrinklist_lock will let shmem_evict_inode grab
> and delete the inode, which will berak the consistency between
> shrinklist_len and shrinklist. The simultaneous deletion of adjacent
> elements in the local list "list" by shmem_unused_huge_shrink and
> shmem_evict_inode will also break the list.
>
> iput must in lock or after lock, but shrinklist_lock is a spinlock
> which can not sleep and iput may sleep.[1]
>
> Fix it by changing shrinklist_lock from spinlock to mutex and moving iput
> into this lock.
>
> [1]. Link: http://lkml.kernel.org/r/20170131093141.GA15899@xxxxxxxxxxxxxxxxxx
> Fixes: 779750d20b93 ("shmem: split huge pages beyond i_size under memory pressure")
> Signed-off-by: Gang Li <ligang.bdlg@xxxxxxxxxxxxx>
> ---
> include/linux/shmem_fs.h | 2 +-
> mm/shmem.c | 16 +++++++---------
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 166158b6e917..65804fd264d0 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -41,7 +41,7 @@ struct shmem_sb_info {
> ino_t next_ino; /* The next per-sb inode number to use */
> ino_t __percpu *ino_batch; /* The next per-cpu inode number to use */
> struct mempolicy *mpol; /* default memory policy for mappings */
> - spinlock_t shrinklist_lock; /* Protects shrinklist */
> + struct mutex shrinklist_mutex;/* Protects shrinklist */
> struct list_head shrinklist; /* List of shinkable inodes */
> unsigned long shrinklist_len; /* Length of shrinklist */
> };
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 18f93c2d68f1..2165a28631c5 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -559,7 +559,7 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> if (list_empty(&sbinfo->shrinklist))
> return SHRINK_STOP;
>
> - spin_lock(&sbinfo->shrinklist_lock);
> + mutex_lock(&sbinfo->shrinklist_mutex);
> list_for_each_safe(pos, next, &sbinfo->shrinklist) {
> info = list_entry(pos, struct shmem_inode_info, shrinklist);
>
> @@ -586,7 +586,6 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> if (!--batch)
> break;
> }
> - spin_unlock(&sbinfo->shrinklist_lock);
>
> list_for_each_safe(pos, next, &to_remove) {
> info = list_entry(pos, struct shmem_inode_info, shrinklist);
> @@ -643,10 +642,9 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
> iput(inode);
It could lead to deadlock, since we could be the last user
of @inode, then shmem_evict_inode() will be called and
try to acquire the mutex lock. Notice that the mutex is already
held here.
Thanks.
> }
>
> - spin_lock(&sbinfo->shrinklist_lock);
> list_splice_tail(&list, &sbinfo->shrinklist);
> sbinfo->shrinklist_len -= removed;
> - spin_unlock(&sbinfo->shrinklist_lock);
> + mutex_unlock(&sbinfo->shrinklist_mutex);
>
> return split;
> }
> @@ -1137,12 +1135,12 @@ static void shmem_evict_inode(struct inode *inode)
> inode->i_size = 0;
> shmem_truncate_range(inode, 0, (loff_t)-1);
> if (!list_empty(&info->shrinklist)) {
> - spin_lock(&sbinfo->shrinklist_lock);
> + mutex_lock(&sbinfo->shrinklist_mutex);
> if (!list_empty(&info->shrinklist)) {
> list_del_init(&info->shrinklist);
> sbinfo->shrinklist_len--;
> }
> - spin_unlock(&sbinfo->shrinklist_lock);
> + mutex_unlock(&sbinfo->shrinklist_mutex);
> }
> while (!list_empty(&info->swaplist)) {
> /* Wait while shmem_unuse() is scanning this inode... */
> @@ -1954,7 +1952,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
> * Part of the huge page is beyond i_size: subject
> * to shrink under memory pressure.
> */
> - spin_lock(&sbinfo->shrinklist_lock);
> + mutex_lock(&sbinfo->shrinklist_mutex);
> /*
> * _careful to defend against unlocked access to
> * ->shrink_list in shmem_unused_huge_shrink()
> @@ -1964,7 +1962,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
> &sbinfo->shrinklist);
> sbinfo->shrinklist_len++;
> }
> - spin_unlock(&sbinfo->shrinklist_lock);
> + mutex_unlock(&sbinfo->shrinklist_mutex);
> }
>
> /*
> @@ -3652,7 +3650,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
> raw_spin_lock_init(&sbinfo->stat_lock);
> if (percpu_counter_init(&sbinfo->used_blocks, 0, GFP_KERNEL))
> goto failed;
> - spin_lock_init(&sbinfo->shrinklist_lock);
> + mutex_init(&sbinfo->shrinklist_mutex);
> INIT_LIST_HEAD(&sbinfo->shrinklist);
>
> sb->s_maxbytes = MAX_LFS_FILESIZE;
> --
> 2.20.1
>
