Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from mpage_submit_folio

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:

quoted

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/ (local)

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);