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

[Joel Fernandes]

On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
> 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)
> =================================================================

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