[PATCH] staging: ashmem: Fix lockdep issue during llseek

[PATCH] staging: ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

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
during a syzcaller test, this patch fixes the issue by releasing the
ashmem_mutex before the call to vfs_llseek, and reacquiring it after.

Cc: Todd Kjos <tkjos@google.com>
Cc: Arve Hjonnevag <arve@android.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 drivers/staging/android/ashmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 0f695df14c9d..248983cf2db1 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -343,7 +343,9 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
 		goto out;
 	}
 
+	mutex_unlock(&ashmem_mutex);
 	ret = vfs_llseek(asma->file, offset, origin);
+	mutex_lock(&ashmem_mutex);
 	if (ret < 0)
 		goto out;
 
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH] ashmem: Fix lockdep issue during llseek

[Al Viro]

On Thu, Jan 25, 2018 at 06:46:49PM -0800, Joel Fernandes wrote:
> 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
> during a syzcaller test, this patch fixes the issue by releasing the
> ashmem_mutex before the call to vfs_llseek, and reacquiring it after.

That looks odd.  If this approach works, what the hell do you need
ashmem_mutex for in ashmem_llseek() in the first place?  What is
it protecting there?

Re: [PATCH] ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

Hi Al,

On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jan 25, 2018 at 06:46:49PM -0800, Joel Fernandes wrote:
>> 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
>> during a syzcaller test, this patch fixes the issue by releasing the
>> ashmem_mutex before the call to vfs_llseek, and reacquiring it after.
>
> That looks odd.  If this approach works, what the hell do you need
> ashmem_mutex for in ashmem_llseek() in the first place?  What is
> it protecting there?

I was just trying to be careful with the least intrusive solution
since I'm not the original author of the driver.

But one usecase for the mutex is with concurrent lseeks, you can end
up with a file->f_pos that is different from the latest update to
asma->file->f_pos. A barrier could fix this it too though. Any
thoughts?

=================================================================
CPU 1                           CPU 2

// vfs_llseek updated
// asma->file->f_pos to X
                                 // vfs_llseek updated
                                 // file->f_pos to Y

                                 asma->file->f_pos updated to Y
asma->file->f_pos updated to X
(lost write)
=================================================================

thanks,

- Joel

Re: [PATCH] ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <joelaf@google.com> wrote:
> Hi Al,
>
> On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Thu, Jan 25, 2018 at 06:46:49PM -0800, Joel Fernandes wrote:
>>> 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
>>> during a syzcaller test, this patch fixes the issue by releasing the
>>> ashmem_mutex before the call to vfs_llseek, and reacquiring it after.
>>
>> That looks odd.  If this approach works, what the hell do you need
>> ashmem_mutex for in ashmem_llseek() in the first place?  What is
>> it protecting there?
>
> I was just trying to be careful with the least intrusive solution
> since I'm not the original author of the driver.
>
> But one usecase for the mutex is with concurrent lseeks, you can end
> up with a file->f_pos that is different from the latest update to
> asma->file->f_pos. A barrier could fix this it too though. Any
> thoughts?
>
> =================================================================
> CPU 1                           CPU 2
>
> // vfs_llseek updated
> // asma->file->f_pos to X
>                                  // vfs_llseek updated
>                                  // file->f_pos to Y
>
>                                  asma->file->f_pos updated to Y
> asma->file->f_pos updated to X
> (lost write)
> =================================================================

I screwed up the explanation here, this is what I had in mind:

=================================================================
CPU 1                           CPU 2

// vfs_llseek updated
// asma->file->f_pos to X
                                 // vfs_llseek updated
                                 // asma->file->f_pos to Y

                                 file->f_pos updated to Y
file->f_pos updated to X
(lost write)
=================================================================


Sorry,

- Joel

Re: [PATCH] ashmem: Fix lockdep issue during llseek

[Al Viro]

On Fri, Jan 26, 2018 at 11:23:47AM -0800, Joel Fernandes wrote:

> I was just trying to be careful with the least intrusive solution
> since I'm not the original author of the driver.

Sure, but that needs a proof that it *is* a solution...

> But one usecase for the mutex is with concurrent lseeks, you can end
> up with a file->f_pos that is different from the latest update to
> asma->file->f_pos. A barrier could fix this it too though. Any
> thoughts?

lseek(2) is serialized against lseek(2) and read(2) on the same struct
file - see fdget_pos() for details.

ashmem_mutex really does look like an overkill - something much lighter
should serve just fine...

Re: [PATCH] ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

Hi Al,

On Fri, Jan 26, 2018 at 11:39 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
[..]
>
>> But one usecase for the mutex is with concurrent lseeks, you can end
>> up with a file->f_pos that is different from the latest update to
>> asma->file->f_pos. A barrier could fix this it too though. Any
>> thoughts?
>
> lseek(2) is serialized against lseek(2) and read(2) on the same struct
> file - see fdget_pos() for details.

Ah right, Ok. Thanks.

> ashmem_mutex really does look like an overkill - something much lighter
> should serve just fine...

There's also the issue of asma->size getting updated from while being
in read ashmem_llseek (although this is a theoretical concern):

if (asma->size == 0) {
  ret = -EINVAL;
  goto out;
}

Which could just use READ_ONCE and WRITE_ONCE instead of the mutex I suppose?

Other than that, I don't see any other issues at the moment with
dropping of the ashmem_mutex from ashmem_llseek.

thanks,

- Joel

Re: [PATCH] staging: ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

On Fri, Feb 16, 2018 at 8:32 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Feb 12, 2018 at 05:01:25PM -0800, Joel Fernandes wrote:
>> 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 unlocking the mutex earlier. Functionally that's Ok since
>> we don't need to protect vfs_llseek.
>>
>> [1] https://patchwork.kernel.org/patch/10185031/
>> [2] https://lkml.org/lkml/2018/1/10/48
>>
>> Cc: Todd Kjos <tkjos@google.com>
>> Cc: Arve Hjonnevag <arve@android.com>
>> Cc: Greg Hackmann <ghackmann@google.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: stable@vger.kernel.org
>> Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com
>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>> ---
>>  drivers/staging/android/ashmem.c | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> Please always properly version your patches, and put what changed below
> the --- line, so I have a hint as to which patch to apply.
> Documentation/SubmittingPatches has the full details of how to do this.
>
> Can you resend me the "latest" version of this patch, so I have a chance
> of getting it right?  :)

Sorry about that :) Fixing now, and will resend. This version you're
replying to is the latest version which is the second version (v2).

- Joel

Re: [PATCH] staging: ashmem: Fix lockdep issue during llseek

[Greg Kroah-Hartman]

On Mon, Feb 12, 2018 at 05:01:25PM -0800, Joel Fernandes wrote:
> 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 unlocking the mutex earlier. Functionally that's Ok since
> we don't need to protect vfs_llseek.
> 
> [1] https://patchwork.kernel.org/patch/10185031/
> [2] https://lkml.org/lkml/2018/1/10/48
> 
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Arve Hjonnevag <arve@android.com>
> Cc: Greg Hackmann <ghackmann@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
>  drivers/staging/android/ashmem.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

Please always properly version your patches, and put what changed below
the --- line, so I have a hint as to which patch to apply.
Documentation/SubmittingPatches has the full details of how to do this.

Can you resend me the "latest" version of this patch, so I have a chance
of getting it right?  :)

thanks,

greg k-h

[PATCH] staging: ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

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 unlocking the mutex earlier. Functionally that's Ok since
we don't need to protect vfs_llseek.

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

Cc: Todd Kjos <tkjos@google.com>
Cc: Arve Hjonnevag <arve@android.com>
Cc: Greg Hackmann <ghackmann@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 drivers/staging/android/ashmem.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 372ce9913e6d..6921f86b4aa1 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -334,24 +334,23 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
 	mutex_lock(&ashmem_mutex);
 
 	if (asma->size == 0) {
-		ret = -EINVAL;
-		goto out;
+		mutex_unlock(&ashmem_mutex);
+		return -EINVAL;
 	}
 
 	if (!asma->file) {
-		ret = -EBADF;
-		goto out;
+		mutex_unlock(&ashmem_mutex);
+		return -EBADF;
 	}
 
+	mutex_unlock(&ashmem_mutex);
+
 	ret = vfs_llseek(asma->file, offset, origin);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/** Copy f_pos from backing file, since f_ops->llseek() sets it */
 	file->f_pos = asma->file->f_pos;
-
-out:
-	mutex_unlock(&ashmem_mutex);
 	return ret;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH] staging: ashmem: Fix lockdep issue during llseek

[Joel Fernandes]

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 unlocking the mutex earlier. Functionally that's Ok since
we don't need to protect vfs_llseek.

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

Cc: Todd Kjos <tkjos@google.com>
Cc: Arve Hjonnevag <arve@android.com>
Cc: Greg Hackmann <ghackmann@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 drivers/staging/android/ashmem.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 7e060f32aaa8..c8b74ae53936 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -335,24 +335,23 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
 	mutex_lock(&ashmem_mutex);
 
 	if (asma->size == 0) {
-		ret = -EINVAL;
-		goto out;
+		mutex_unlock(&ashmem_mutex);
+		return -EINVAL;
 	}
 
 	if (!asma->file) {
-		ret = -EBADF;
-		goto out;
+		mutex_unlock(&ashmem_mutex);
+		return -EBADF;
 	}
 
+	mutex_unlock(&ashmem_mutex);
+
 	ret = vfs_llseek(asma->file, offset, origin);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/** Copy f_pos from backing file, since f_ops->llseek() sets it */
 	file->f_pos = asma->file->f_pos;
-
-out:
-	mutex_unlock(&ashmem_mutex);
 	return ret;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog