Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <CAJWu+oo+Y4eNTX+vMceYTB-FF5A190OHP7yPKkDGYH-jTqSsmQ@mail.gmail.com>
References: <20180126024649.200330-1-joelaf@google.com> <20180126031346.GW13338@ZenIV.linux.org.uk>
 <CAJWu+oo+Y4eNTX+vMceYTB-FF5A190OHP7yPKkDGYH-jTqSsmQ@mail.gmail.com>
From:   Joel Fernandes <joelaf@google.com>
Date:   Fri, 26 Jan 2018 11:25:14 -0800
Message-ID: <CAJWu+oqExkyK+1Yp5VMH1P9p1OTkkhFtY9P1hNJJQ1u_Ds8reg@mail.gmail.com>
Subject: Re: [PATCH] ashmem: Fix lockdep issue during llseek
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     LKML <linux-kernel@vger.kernel.org>, Todd Kjos <tkjos@google.com>,
        Arve Hjonnevag <arve@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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