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

From: Joel Fernandes
Date: Fri Jan 26 2018 - 14:23:55 EST


Hi Al,

On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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