[PATCH] shmem: fix possible deadlocks on shmlock_user_lock

[PATCH] shmem: fix possible deadlocks on shmlock_user_lock

[Hugh Dickins]

Recent commit 71725ed10c40 ("mm: huge tmpfs: try to split_huge_page()
when punching hole") has allowed syzkaller to probe deeper, uncovering
a long-standing lockdep issue between the irq-unsafe shmlock_user_lock,
the irq-safe xa_lock on mapping->i_pages, and shmem inode's info->lock
which nests inside xa_lock (or tree_lock) since 4.8's shmem_uncharge().

user_shm_lock(), servicing SysV shmctl(SHM_LOCK), wants shmlock_user_lock
while its caller shmem_lock() holds info->lock with interrupts disabled;
but hugetlbfs_file_setup() calls user_shm_lock() with interrupts enabled,
and might be interrupted by a writeback endio wanting xa_lock on i_pages.
This may not risk an actual deadlock, since shmem inodes do not take part
in writeback accounting, but there are several easy ways to avoid it.

Requiring interrupts disabled for shmlock_user_lock would be easy,
but it's a high-level global lock for which that seems inappropriate.
Instead, recall that the use of info->lock to guard info->flags in
shmem_lock() dates from pre-3.1 days, when races with SHMEM_PAGEIN and
SHMEM_TRUNCATE could occur: nowadays it serves no purpose, the only flag
added or removed is VM_LOCKED itself, and calls to shmem_lock() an inode
are already serialized by the caller.  Take info->lock out of the chain
and the possibility of deadlock or lockdep warning goes away.

Reported-by: syzbot+c8a8197c8852f566b9d9@syzkaller.appspotmail.com
Link: https://lore.kernel.org/lkml/000000000000e5838c05a3152f53@google.com/
Reported-by: syzbot+40b71e145e73f78f81ad@syzkaller.appspotmail.com
Link: https://lore.kernel.org/lkml/0000000000003712b305a331d3b1@google.com/
Fixes: 4595ef88d136 ("shmem: make shmem_inode_info::lock irq-safe")
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
---

 mm/shmem.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- 5.7-rc1/mm/shmem.c	2020-04-11 12:58:26.415524805 -0700
+++ linux/mm/shmem.c	2020-04-16 11:04:06.729738730 -0700
@@ -2179,7 +2179,11 @@ int shmem_lock(struct file *file, int lo
 	struct shmem_inode_info *info = SHMEM_I(inode);
 	int retval = -ENOMEM;
 
-	spin_lock_irq(&info->lock);
+	/*
+	 * What serializes the accesses to info->flags?
+	 * ipc_lock_object() when called from shmctl_do_lock(),
+	 * no serialization needed when called from shm_destroy().
+	 */
 	if (lock && !(info->flags & VM_LOCKED)) {
 		if (!user_shm_lock(inode->i_size, user))
 			goto out_nomem;
@@ -2194,7 +2198,6 @@ int shmem_lock(struct file *file, int lo
 	retval = 0;
 
 out_nomem:
-	spin_unlock_irq(&info->lock);
 	return retval;
 }
 

Re: [PATCH] shmem: fix possible deadlocks on shmlock_user_lock

[Yang Shi]

On 4/16/20 5:11 PM, Hugh Dickins wrote:
> Recent commit 71725ed10c40 ("mm: huge tmpfs: try to split_huge_page()
> when punching hole") has allowed syzkaller to probe deeper, uncovering
> a long-standing lockdep issue between the irq-unsafe shmlock_user_lock,
> the irq-safe xa_lock on mapping->i_pages, and shmem inode's info->lock
> which nests inside xa_lock (or tree_lock) since 4.8's shmem_uncharge().
>
> user_shm_lock(), servicing SysV shmctl(SHM_LOCK), wants shmlock_user_lock
> while its caller shmem_lock() holds info->lock with interrupts disabled;
> but hugetlbfs_file_setup() calls user_shm_lock() with interrupts enabled,
> and might be interrupted by a writeback endio wanting xa_lock on i_pages.
> This may not risk an actual deadlock, since shmem inodes do not take part
> in writeback accounting, but there are several easy ways to avoid it.
>
> Requiring interrupts disabled for shmlock_user_lock would be easy,
> but it's a high-level global lock for which that seems inappropriate.
> Instead, recall that the use of info->lock to guard info->flags in
> shmem_lock() dates from pre-3.1 days, when races with SHMEM_PAGEIN and
> SHMEM_TRUNCATE could occur: nowadays it serves no purpose, the only flag
> added or removed is VM_LOCKED itself, and calls to shmem_lock() an inode
> are already serialized by the caller.  Take info->lock out of the chain
> and the possibility of deadlock or lockdep warning goes away.
>
> Reported-by: syzbot+c8a8197c8852f566b9d9@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/lkml/000000000000e5838c05a3152f53@google.com/
> Reported-by: syzbot+40b71e145e73f78f81ad@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/lkml/0000000000003712b305a331d3b1@google.com/
> Fixes: 4595ef88d136 ("shmem: make shmem_inode_info::lock irq-safe")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>
>   mm/shmem.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

Acked-by: Yang Shi <yang.shi@linux.alibaba.com>

>
> --- 5.7-rc1/mm/shmem.c	2020-04-11 12:58:26.415524805 -0700
> +++ linux/mm/shmem.c	2020-04-16 11:04:06.729738730 -0700
> @@ -2179,7 +2179,11 @@ int shmem_lock(struct file *file, int lo
>   	struct shmem_inode_info *info = SHMEM_I(inode);
>   	int retval = -ENOMEM;
>   
> -	spin_lock_irq(&info->lock);
> +	/*
> +	 * What serializes the accesses to info->flags?
> +	 * ipc_lock_object() when called from shmctl_do_lock(),
> +	 * no serialization needed when called from shm_destroy().
> +	 */
>   	if (lock && !(info->flags & VM_LOCKED)) {
>   		if (!user_shm_lock(inode->i_size, user))
>   			goto out_nomem;
> @@ -2194,7 +2198,6 @@ int shmem_lock(struct file *file, int lo
>   	retval = 0;
>   
>   out_nomem:
> -	spin_unlock_irq(&info->lock);
>   	return retval;
>   }
>