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

[Joel Fernandes <joelaf@google.com>]

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