Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

netfs: Fix unbuffered write error handling

If all the subrequests in an unbuffered write stream fail, the subrequest
collector doesn't update the stream->transferred value and it retains its
initial LONG_MAX value. Unfortunately, if all active streams fail, then we
take the smallest value of { LONG_MAX, LONG_MAX, ... } as the value to set
in wreq->transferred - which is then returned from ->write_iter().

LONG_MAX was chosen as the initial value so that all the streams can be
quickly assessed by taking the smallest value of all stream->transferred -
but this only works if we've set any of them.

Fix this by adding a flag to indicate whether the value in
stream->transferred is valid and checking that when we integrate the
values. stream->transferred can then be initialised to zero.

This was found by running the generic/750 xfstest against cifs with
cache=none. It splices data to the target file. Once (if) it has used up
all the available scratch space, the writes start failing with ENOSPC.
This causes ->write_iter() to fail. However, it was returning
wreq->transferred, i.e. LONG_MAX, rather than an error (because it thought
the amount transferred was non-zero) and iter_file_splice_write() would
then try to clean up that amount of pipe bufferage - leading to an oops
when it overran. The kernel log showed:

CIFS: VFS: Send error in write = -28

followed by:

BUG: kernel NULL pointer dereference, address: 0000000000000008

with:

RIP: 0010:iter_file_splice_write+0x3a4/0x520
do_splice+0x197/0x4e0

or:

RIP: 0010:pipe_buf_release (include/linux/pipe_fs_i.h:282)
iter_file_splice_write (fs/splice.c:755)

Also put a warning check into splice to announce if ->write_iter() returned
that it had written more than it was asked to.

Fixes: 288ace2f57c9 ("netfs: New writeback implementation")
Reported-by: Xiaoli Feng <fengxiaoli0714@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220445
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/915443.1755207950@warthog.procyon.org.uk
cc: Paulo Alcantara <pc@manguebit.org>
cc: Steve French <sfrench@samba.org>
cc: Shyam Prasad N <sprasad@microsoft.com>
cc: netfs@lists.linux.dev
cc: linux-cifs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>

authored by

David Howells and committed by
Christian Brauner
a3de58b1 b5ca8892

+17 -5
+3 -1
fs/netfs/read_collect.c
··· 281 281 } else if (test_bit(NETFS_RREQ_SHORT_TRANSFER, &rreq->flags)) { 282 282 notes |= MADE_PROGRESS; 283 283 } else { 284 - if (!stream->failed) 284 + if (!stream->failed) { 285 285 stream->transferred += transferred; 286 + stream->transferred_valid = true; 287 + } 286 288 if (front->transferred < front->len) 287 289 set_bit(NETFS_RREQ_SHORT_TRANSFER, &rreq->flags); 288 290 notes |= MADE_PROGRESS;
+8 -2
fs/netfs/write_collect.c
··· 254 254 if (front->start + front->transferred > stream->collected_to) { 255 255 stream->collected_to = front->start + front->transferred; 256 256 stream->transferred = stream->collected_to - wreq->start; 257 + stream->transferred_valid = true; 257 258 notes |= MADE_PROGRESS; 258 259 } 259 260 if (test_bit(NETFS_SREQ_FAILED, &front->flags)) { ··· 357 356 { 358 357 struct netfs_inode *ictx = netfs_inode(wreq->inode); 359 358 size_t transferred; 359 + bool transferred_valid = false; 360 360 int s; 361 361 362 362 _enter("R=%x", wreq->debug_id); ··· 378 376 continue; 379 377 if (!list_empty(&stream->subrequests)) 380 378 return false; 381 - if (stream->transferred < transferred) 379 + if (stream->transferred_valid && 380 + stream->transferred < transferred) { 382 381 transferred = stream->transferred; 382 + transferred_valid = true; 383 + } 383 384 } 384 385 385 386 /* Okay, declare that all I/O is complete. */ 386 - wreq->transferred = transferred; 387 + if (transferred_valid) 388 + wreq->transferred = transferred; 387 389 trace_netfs_rreq(wreq, netfs_rreq_trace_write_done); 388 390 389 391 if (wreq->io_streams[1].active &&
+2 -2
fs/netfs/write_issue.c
··· 118 118 wreq->io_streams[0].prepare_write = ictx->ops->prepare_write; 119 119 wreq->io_streams[0].issue_write = ictx->ops->issue_write; 120 120 wreq->io_streams[0].collected_to = start; 121 - wreq->io_streams[0].transferred = LONG_MAX; 121 + wreq->io_streams[0].transferred = 0; 122 122 123 123 wreq->io_streams[1].stream_nr = 1; 124 124 wreq->io_streams[1].source = NETFS_WRITE_TO_CACHE; 125 125 wreq->io_streams[1].collected_to = start; 126 - wreq->io_streams[1].transferred = LONG_MAX; 126 + wreq->io_streams[1].transferred = 0; 127 127 if (fscache_resources_valid(&wreq->cache_resources)) { 128 128 wreq->io_streams[1].avail = true; 129 129 wreq->io_streams[1].active = true;
+3
fs/splice.c
··· 739 739 sd.pos = kiocb.ki_pos; 740 740 if (ret <= 0) 741 741 break; 742 + WARN_ONCE(ret > sd.total_len - left, 743 + "Splice Exceeded! ret=%zd tot=%zu left=%zu\n", 744 + ret, sd.total_len, left); 742 745 743 746 sd.num_spliced += ret; 744 747 sd.total_len -= ret;
+1
include/linux/netfs.h
··· 150 150 bool active; /* T if stream is active */ 151 151 bool need_retry; /* T if this stream needs retrying */ 152 152 bool failed; /* T if this stream failed */ 153 + bool transferred_valid; /* T is ->transferred is valid */ 153 154 }; 154 155 155 156 /*