Messages in this thread | | | From | Joel Fernandes <> | Date | Fri, 26 Jan 2018 11:25:14 -0800 | Subject | Re: [PATCH] ashmem: Fix lockdep issue during llseek |
| |
On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <joelaf@google.com> wrote: > 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) > =================================================================
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
| |