On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <joe...@google.com> wrote:
> Hi Al,
>
> On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <v...@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
