Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Jun 2023 02:58:15 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     Baokun Li <libaokun1@huawei.com>, linux-ext4@vger.kernel.org,
        adilger.kernel@dilger.ca, jack@suse.cz, ritesh.list@gmail.com,
        linux-kernel@vger.kernel.org, jun.nie@linaro.org,
        ebiggers@kernel.org, yi.zhang@huawei.com, yangerkun@huawei.com,
        yukuai3@huawei.com,
        syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com,
        stable@vger.kernel.org
Subject: Re: [PATCH v2] ext4: fix race condition between buffer write and
 page_mkwrite
Message-ID: <ZH1BN+H1/Sa4eLQ4@casper.infradead.org>
References: <20230530134405.322194-1-libaokun1@huawei.com>
 <20230604030445.GF1128744@mit.edu>
 <20230604210821.GA1257572@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230604210821.GA1257572@mit.edu>
Precedence: bulk

On Sun, Jun 04, 2023 at 05:08:21PM -0400, Theodore Ts'o wrote:
> On Sat, Jun 03, 2023 at 11:04:45PM -0400, Theodore Ts'o wrote:
> > I tried testing to see if this fixed [1], and it appears to be
> > triggering a lockdep warning[2] at this line in the patch:
> > 
> > [1] https://syzkaller.appspot.com/bug?extid=f4582777a19ec422b517
> > [2] https://syzkaller.appspot.com/x/report.txt?x=17260843280000
> 
> Looking at this more closely, the fundamental problem is by the time
> ext4_file_mmap() is called, the mm layer has already taken
> current->mm->mmap_lock, and when we try to take the inode_lock, this
> causes locking ordering problems with how buffered write path works,
> which take the inode_lock first, and then in some cases, may end up
> taking the mmap_lock if there is a page fault for the buffer used for
> the buffered write.
> 
> If we're going to stick with the approach in this patch, I think what
> we would need is to add a pre_mmap() function to file_operations
> struct, which would get called by the mmap path *before* taking
> current->mm->mmap_lock, so we can do the inline conversion before we
> take the mmap_lock.
> 
> I'm not sure how the mm folks would react to such a proposal, though.
> I could be seen as a bit hacky, and it's not clear that any file
> system other than ext4 would need something like this.  Willy, as
> someone who does a lot of work in both mm and fs worlds --- I'm
> curious what you think about this idea?

I'm probably missing something here, but why do we need to convert inline
data in page_mkwrite?  mmap() can't change i_size (stores past i_size are
discarded), so we should be able to simply copy the data from the page
cache into the inode and write the inode when it comes to writepages()
time.

Unless somebody does a truncate() or write() that expands i_size, but we
should be able to do the conversion then without the mmap_lock held.  No?
I'm not too familiar with inline data.