Skip to contentSkip to conversation

staging: ashmem: Fix lockdep issue during llseek

PATCH In review Joel Fernandes 2 participants 6 messages Jan 26, 2018 Staging

lore
Share & tools
lore

Joel Fernandes

author
#0 lore raw

PATCH 1/1 staging: 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 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 <[email protected]>
Cc Arve Hjonnevag <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reported-by [email protected]
Signed-off-by Joel Fernandes <[email protected]>
---
 drivers/staging/android/ashmem.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/staging/android/ashmem.c +2 -0
index 0f695df14c9d..248983cf2db1 100644
@@ -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;
Signature · 1 line
2.16.0.rc1.238.g530d649a79-goog
#1 lore raw

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

Quoted 25 lines · 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)

Quoted 3 lines

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?

author
#2 lore raw

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

Hi Al,

Quoted 26 lines · Al Viro
Quoted 25 lines · 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)

Quoted 7 lines
Quoted 3 lines

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

author
#3 lore raw

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

Quoted 50 lines · Joel Fernandes

Hi Al,

Quoted 26 lines · Al Viro
Quoted 25 lines · 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)

Quoted 7 lines
Quoted 3 lines

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

Quoted 7 lines
                                 // 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

#4 lore raw

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

In reply to Joel Fernandes
Quoted 2 lines · Joel Fernandes

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...

Quoted 4 lines

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...

author
#5 lore raw

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

Hi Al,

On Fri, Jan 26, 2018 at 11:39 AM, Al Viro <[email protected]> wrote:
[..]

Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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