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 causing a lockdep splat [2] during a syzcaller test, this patch fixes the issue by unlocking the mutex earlier. Functionally that's Ok since we don't need to protect vfs_llseek.
[1] https://patchwork.kernel.org/patch/10185031/ [2] https://lkml.org/lkml/2018/1/10/48
Cc: Todd Kjos tkjos@google.com Cc: Arve Hjonnevag arve@android.com Cc: Greg Hackmann ghackmann@google.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com Signed-off-by: Joel Fernandes joelaf@google.com --- Changes since first version: Don't relock after vfs call since its not needed. Only reason we lock is to protect races with asma->file. https://patchwork.kernel.org/patch/10185031/
drivers/staging/android/ashmem.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index bbdc53b686dd..b330e86b3a49 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -326,24 +326,23 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin) mutex_lock(&ashmem_mutex);
if (asma->size == 0) { - ret = -EINVAL; - goto out; + mutex_unlock(&ashmem_mutex); + return -EINVAL; }
if (!asma->file) { - ret = -EBADF; - goto out; + mutex_unlock(&ashmem_mutex); + return -EBADF; }
+ mutex_unlock(&ashmem_mutex); + ret = vfs_llseek(asma->file, offset, origin); if (ret < 0) - goto out; + return ret;
/** Copy f_pos from backing file, since f_ops->llseek() sets it */ file->f_pos = asma->file->f_pos; - -out: - mutex_unlock(&ashmem_mutex); return ret; }
On Fri, Feb 16, 2018 at 11:02:01AM -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 causing a lockdep splat [2] during a syzcaller test, this patch fixes the issue by unlocking the mutex earlier. Functionally that's Ok since we don't need to protect vfs_llseek.
[1] https://patchwork.kernel.org/patch/10185031/ [2] https://lkml.org/lkml/2018/1/10/48
Cc: Todd Kjos tkjos@google.com Cc: Arve Hjonnevag arve@android.com Cc: Greg Hackmann ghackmann@google.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com Signed-off-by: Joel Fernandes joelaf@google.com
Changes since first version: Don't relock after vfs call since its not needed. Only reason we lock is to protect races with asma->file. https://patchwork.kernel.org/patch/10185031/
I'd like some acks from others before I take this patch.
Joel, did the original reporter say this patch solved their issue or not? For some reason I didn't think it worked properly...
thanks,
greg k-h
(reposting in plain text, sorry for the previous HTML email, I should have not posted from the Phone)
On Thu, Feb 22, 2018 at 5:48 AM, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Feb 16, 2018 at 11:02:01AM -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 causing a lockdep splat [2] during a syzcaller test, this patch fixes the issue by unlocking the mutex earlier. Functionally that's Ok since we don't need to protect vfs_llseek.
[1] https://patchwork.kernel.org/patch/10185031/ [2] https://lkml.org/lkml/2018/1/10/48
Cc: Todd Kjos tkjos@google.com Cc: Arve Hjonnevag arve@android.com Cc: Greg Hackmann ghackmann@google.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com Signed-off-by: Joel Fernandes joelaf@google.com
Changes since first version: Don't relock after vfs call since its not needed. Only reason we lock is to protect races with asma->file. https://patchwork.kernel.org/patch/10185031/
I'd like some acks from others before I take this patch.
GregH, Todd, could you provide Acks?
Joel, did the original reporter say this patch solved their issue or not? For some reason I didn't think it worked properly...
That's a different but similar issue: https://patchwork.kernel.org/patch/10202127/. That was related to RECLAIM_FS lockdep recursion. That was posted as an RFC unlike this one, and its still being investigated since Miles reported that the lockdep inode annotation doesn't fix the issue.
This one on the other hand has a straightforward fix, and was verified by the syzbot.
I can see why its easy to confuse the two issues.
Thanks,
- Joel
Ack. I confirmed that the syzbot could not reproduce the issue with this patch.
On Thu, Feb 22, 2018 at 1:02 PM, Joel Fernandes joelaf@google.com wrote:
(reposting in plain text, sorry for the previous HTML email, I should have not posted from the Phone)
On Thu, Feb 22, 2018 at 5:48 AM, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Feb 16, 2018 at 11:02:01AM -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 causing a lockdep splat [2] during a syzcaller test, this patch fixes the issue by unlocking the mutex earlier. Functionally that's Ok since we don't need to protect vfs_llseek.
[1] https://patchwork.kernel.org/patch/10185031/ [2] https://lkml.org/lkml/2018/1/10/48
Cc: Todd Kjos tkjos@google.com Cc: Arve Hjonnevag arve@android.com Cc: Greg Hackmann ghackmann@google.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com Signed-off-by: Joel Fernandes joelaf@google.com
Changes since first version: Don't relock after vfs call since its not needed. Only reason we lock is to protect races with asma->file. https://patchwork.kernel.org/patch/10185031/
I'd like some acks from others before I take this patch.
GregH, Todd, could you provide Acks?
Joel, did the original reporter say this patch solved their issue or not? For some reason I didn't think it worked properly...
That's a different but similar issue: https://patchwork.kernel.org/patch/10202127/. That was related to RECLAIM_FS lockdep recursion. That was posted as an RFC unlike this one, and its still being investigated since Miles reported that the lockdep inode annotation doesn't fix the issue.
This one on the other hand has a straightforward fix, and was verified by the syzbot.
I can see why its easy to confuse the two issues.
Thanks,
- Joel
On 02/22/2018 01:02 PM, Joel Fernandes wrote:
(reposting in plain text, sorry for the previous HTML email, I should have not posted from the Phone)
On Thu, Feb 22, 2018 at 5:48 AM, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Feb 16, 2018 at 11:02:01AM -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 causing a lockdep splat [2] during a syzcaller test, this patch fixes the issue by unlocking the mutex earlier. Functionally that's Ok since we don't need to protect vfs_llseek.
[1] https://patchwork.kernel.org/patch/10185031/ [2] https://lkml.org/lkml/2018/1/10/48
Cc: Todd Kjos tkjos@google.com Cc: Arve Hjonnevag arve@android.com Cc: Greg Hackmann ghackmann@google.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Reported-by: syzbot+8ec30bb7bf1a981a2012@syzkaller.appspotmail.com Signed-off-by: Joel Fernandes joelaf@google.com
Changes since first version: Don't relock after vfs call since its not needed. Only reason we lock is to protect races with asma->file. https://patchwork.kernel.org/patch/10185031/
I'd like some acks from others before I take this patch.
GregH, Todd, could you provide Acks?
Acked-by: Greg Hackmann ghackmann@google.com
linux-stable-mirror@lists.linaro.org
-
Greg Hackmann -
Greg Kroah-Hartman -
Joel Fernandes -
Todd Kjos