Re: possible deadlock in shmem_file_llseek

[Joel Fernandes]

On Wed, Jan 24, 2018 at 10:40 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> On Wed, Jan 24, 2018 at 6:47 PM, Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
>>
>> #syz test: https://github.com/joelagnel/linux.git test-ashmem

Here's an updated patch. Just wanted to test it once more.

thanks,

- Joel
From e7d73362ed378499795f27e2da6184d0b242b89c Mon Sep 17 00:00:00 2001
From: Joel Fernandes <joelaf@xxxxxxxxxx>
Date: Wed, 24 Jan 2018 09:47:23 -0800
Subject: [PATCH] ashmem: Fix lockdep issue during llseek

ashmem_mutex create a chain of dependencies like so:

(1)
mmap syscall ->
  mmap_sem ->  (acquired)
  ashmem_mmap
  ashmem_mutex (try to acquire)
  (block)

(2)
llseek syscall ->
  ashmem_llseek ->
  ashmem_mutex ->  (acquired)
  inode_lock ->
  inode->i_rwsem (try to acquire)
  (block)

(3)
getdents ->
  iterate_dir ->
  inode_lock ->
  inode->i_rwsem   (acquired)
  copy_to_user ->
  mmap_sem         (try to acquire)

There is a lock ordering created between mmap_sem and inode->i_rwsem causing a
lockdep splat [2] during a syzcaller test, this patch fixes the issue by
removing the use of the mutex.

The mutex isn't needed as llseeks are synchronized with other llseeks
and reads due to fdget_pos as Al mentioned [1]. Further asma->file
and asma->size don't change once they're setup, and there's a unique
asma->file per file.

[1] https://patchwork.kernel.org/patch/10185031/
[2] https://lkml.org/lkml/2018/1/10/48

Cc: Todd Kjos <tkjos@xxxxxxxxxx>
Cc: Arve Hjonnevag <arve@xxxxxxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Reported-by: syzbot+8ec30bb7bf1a981a2012@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Joel Fernandes <joelaf@xxxxxxxxxx>
---
 drivers/staging/android/ashmem.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 0f695df14c9d..8c9c69a93fa4 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -331,8 +331,6 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
 	struct ashmem_area *asma = file->private_data;
 	int ret;
 
-	mutex_lock(&ashmem_mutex);
-
 	if (asma->size == 0) {
 		ret = -EINVAL;
 		goto out;
@@ -351,7 +349,6 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
 	file->f_pos = asma->file->f_pos;
 
 out:
-	mutex_unlock(&ashmem_mutex);
 	return ret;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog