Re: [[ EXT ]] [BUG REPORT] cifs/smb data corruption when writing, x86_64, kernel 6.6.71

[David Howells <dhowells@redhat.com>]

Okay, the patch isn't good from a quick scan of it.

> +			if (folio_test_writeback(folio)) {
> +				/*
> +				 * For data-integrity syscalls (fsync(), msync()) we must wait for
> +				 * the I/O to complete on the page.
> +				 * For other cases (!sync), we can just skip this page, even if
> +				 * it's dirty.
> +				 */
> +				if (!sync) {
> +					stop = false;
> +					goto unlock_next;
> +				} else {
> +					folio_wait_writeback(folio);

You can't sleep here.  The RCU read lock is held.  There's no actual need to
sleep here anyway - you can just stop and leave the function (well, set
stop=true and break so that the accumulated batch is processed).

The way the code is meant to work is that cifs_write_back_from_locked_folio()
locks and waits for the first folio, then calls cifs_extend_writeback() to add
more folios to the writeout - if it doesn't need to wait for them.  You cannot
skip any folios as the set has to be contiguous.  If you skip one, you'll
corrupt the file.

Once a set of folios has been dispatched, cifs_writepages_begin() *should*
begin with the next folio that hasn't been sent - quite possibly one just
rejected by cifs_extend_writeback().  But at this point
cifs_write_back_from_locked_folio() will wait for it.

This should[*] work correctly, even in sync mode, because it should eventually
wait for any folio that's already undergoing writeback - though it might be
less efficient because if there are competing writebacks, they may end up
forcing each other to produce very small writes (there's no
writeback-vs-writeback locking apart from the individual folio locks).

[*] At least as far as the design goes; that's not to say there isn't a bug in
    the implementation.

That said, in sync mode, you might actually want cifs_extend_writeback() to
wait - but in that case, you have to drop the RCU read lock before you do the
wait and then reset the iteration correctly... and beware that doing that
might advance[**] the iterator state.

[**] It's possible that this is the actual cause of the bug - and that we're
     skipping the rejected folio because the xa_state isn't been correctly
     rewound.

David