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:   Sun, 11 Jun 2023 15:15:54 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Disha Goel <disgoel@linux.ibm.com>
Subject: Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from
 mpage_submit_folio
Message-ID: <ZIXXGhvfJEaKDIXC@casper.infradead.org>
References: <cover.1684122756.git.ritesh.list@gmail.com>
 <74182f5607ccfc3b1e7f08737fcb3442b42a2124.1684122756.git.ritesh.list@gmail.com>
 <20230611055831.GF1436857@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230611055831.GF1436857@mit.edu>
Precedence: bulk

On Sun, Jun 11, 2023 at 01:58:31AM -0400, Theodore Ts'o wrote:
> On Mon, May 15, 2023 at 04:10:41PM +0530, Ritesh Harjani (IBM) wrote:
> > mpage_submit_folio() was converted to take folio. Even though
> > folio_size() in ext4 as of now is PAGE_SIZE, but it's better to
> > remove that assumption which I am assuming is a missed left over from
> > patch[1].
> > 
> > [1]: https://lore.kernel.org/linux-ext4/20230324180129.1220691-7-willy@infradead.org/
> > 
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> 
> I didn't notice this right away, because the failure is not 100%
> reliable, but this commit will sometimes cause "kvm-xfstests -c
> ext4/encrypt generic/068" to crash.  Reverting the patch fixes the
> problem, so I plan to drop this patch from my tree.

Hrm.  Well, let's think about how this can go wrong:

@@ -1885,7 +1885,7 @@ static int mpage_submit_folio(struct mpage_da_data *mpd,
+struct folio *folio)
        len = folio_size(folio);
        if (folio_pos(folio) + len > size &&
            !ext4_verity_in_progress(mpd->inode))
-               len = size & ~PAGE_MASK;
+        	len = size - folio_pos(folio);
        err = ext4_bio_write_folio(&mpd->io_submit, folio, len);
        if (!err)
                mpd->wbc->nr_to_write--;

Just off-camera is:

        size = i_size_read(mpd->inode);

Now, nothing is preventing i_size to be truncated to far below this
folio, right?  So if that happened before this patch, we'd write some
randomly sized fragment of the page.  Now we'll get a negative result
... which is assigned to size_t, so is exabytes in size.

So do we care if we write a random fragment of a page after a truncate?
If so, we should add:

	if (folio_pos(folio) >= size)
		return 0; /* Do we need to account nr_to_write? */

If we simply don't care that we're doing a spurious write, then we can
do something like:

-		len = size & ~PAGE_MASK;
+		len = size & (len - 1);