Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Mon, Jun 05, 2023 at 11:37:00PM +0200, Richard Weinberger wrote:
> > -	addr = kmap(page);
> > -	block = page->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> > +	addr = kmap_local_folio(folio, offset);
> > +	block = folio->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> > 	i = 0;
> > -	while (len) {
> > -		blen = min_t(int, len, UBIFS_BLOCK_SIZE);
> > +	for (;;) {
> 
> This change will cause a file system corruption.
> If len is zero (it can be) then a zero length data node will be written.
> The while(len) made sure that upon zero length nothing is written.

I don't see how 'len' can be 0.  len is modified each time around the
loop, and if it's decremented to 0, we break.  So you must be referring
to a case where the caller of do_writepage passes 0.

There are three callers of do_writepage, two in ubifs_writepage():

        int err, len = folio_size(folio);
...
        if (folio_pos(folio) + len < i_size) {
...
                return do_writepage(folio, len);

len is folio_size(), which is not 0.

        len = offset_in_folio(folio, i_size);

Here, we know that len is not 0.  We already tested earlier:
        if (folio_pos(folio) >= i_size) {

so we know that i_size > folio_pos() and i_size < folio_pos() +
folio_size().  Actually, I should make this more explicit:

	len = i_size - folio_pos(folio);

Now it should be clear that len cannot be zero.

The third caller is do_truncation():

        loff_t old_size = inode->i_size, new_size = attr->ia_size;
        int offset = new_size & (UBIFS_BLOCK_SIZE - 1), budgeted = 1;
        if (offset) {
                pgoff_t index = new_size >> PAGE_SHIFT;
                                       offset = offset_in_folio(folio,
                                                        new_size);
                                err = do_writepage(folio, offset);

It's not large-folio-safe, but it's definitely not 0.

Did I miss something?