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>

diff --git a/fs/netfs/read_collect.c b/fs/netfs/read_collect.c
index 3e804da1e1eb100fc9d7fb68de65b11f095022e9..a95e7aadafd072edbb0f1743e0020eaf65b7d1ed 100644 (file)
--- a/fs/netfs/read_collect.c
+++ b/fs/netfs/read_collect.c
@@ -281,8 +281,10 @@ reassess:
                } else if (test_bit(NETFS_RREQ_SHORT_TRANSFER, &rreq->flags)) {
                        notes |= MADE_PROGRESS;
                } else {
-                       if (!stream->failed)
+                       if (!stream->failed) {
                                stream->transferred += transferred;
+                               stream->transferred_valid = true;
+                       }
                        if (front->transferred < front->len)
                                set_bit(NETFS_RREQ_SHORT_TRANSFER, &rreq->flags);
                        notes |= MADE_PROGRESS;

diff --git a/fs/netfs/write_collect.c b/fs/netfs/write_collect.c
index 0f3a36852a4dc13cd71ca24dc06fdb838dadb982..cbf3d9194c7bf6682d55ca86a9906a2f4b128da0 100644 (file)
--- a/fs/netfs/write_collect.c
+++ b/fs/netfs/write_collect.c
@@ -254,6 +254,7 @@ reassess_streams:
                        if (front->start + front->transferred > stream->collected_to) {
                                stream->collected_to = front->start + front->transferred;
                                stream->transferred = stream->collected_to - wreq->start;
+                               stream->transferred_valid = true;
                                notes |= MADE_PROGRESS;
                        }
                        if (test_bit(NETFS_SREQ_FAILED, &front->flags)) {
@@ -356,6 +357,7 @@ bool netfs_write_collection(struct netfs_io_request *wreq)
 {
        struct netfs_inode *ictx = netfs_inode(wreq->inode);
        size_t transferred;
+       bool transferred_valid = false;
        int s;
 
        _enter("R=%x", wreq->debug_id);
@@ -376,12 +378,16 @@ bool netfs_write_collection(struct netfs_io_request *wreq)
                        continue;
                if (!list_empty(&stream->subrequests))
                        return false;
-               if (stream->transferred < transferred)
+               if (stream->transferred_valid &&
+                   stream->transferred < transferred) {
                        transferred = stream->transferred;
+                       transferred_valid = true;
+               }
        }
 
        /* Okay, declare that all I/O is complete. */
-       wreq->transferred = transferred;
+       if (transferred_valid)
+               wreq->transferred = transferred;
        trace_netfs_rreq(wreq, netfs_rreq_trace_write_done);
 
        if (wreq->io_streams[1].active &&

diff --git a/fs/netfs/write_issue.c b/fs/netfs/write_issue.c
index 50bee2c4130d1eacf9899317a41abd836aa27a42..0584cba1a0439207e53119139141afd145802ba8 100644 (file)
--- a/fs/netfs/write_issue.c
+++ b/fs/netfs/write_issue.c
@@ -118,12 +118,12 @@ struct netfs_io_request *netfs_create_write_req(struct address_space *mapping,
        wreq->io_streams[0].prepare_write       = ictx->ops->prepare_write;
        wreq->io_streams[0].issue_write         = ictx->ops->issue_write;
        wreq->io_streams[0].collected_to        = start;
-       wreq->io_streams[0].transferred         = LONG_MAX;
+       wreq->io_streams[0].transferred         = 0;
 
        wreq->io_streams[1].stream_nr           = 1;
        wreq->io_streams[1].source              = NETFS_WRITE_TO_CACHE;
        wreq->io_streams[1].collected_to        = start;
-       wreq->io_streams[1].transferred         = LONG_MAX;
+       wreq->io_streams[1].transferred         = 0;
        if (fscache_resources_valid(&wreq->cache_resources)) {
                wreq->io_streams[1].avail       = true;
                wreq->io_streams[1].active      = true;

diff --git a/fs/splice.c b/fs/splice.c
index 4d6df083e0c06a774fd421806a3e40de6d32447f..f5094b6d00a09f0f294d4aa4a5460ddacd053baa 100644 (file)
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -739,6 +739,9 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
                sd.pos = kiocb.ki_pos;
                if (ret <= 0)
                        break;
+               WARN_ONCE(ret > sd.total_len - left,
+                         "Splice Exceeded! ret=%zd tot=%zu left=%zu\n",
+                         ret, sd.total_len, left);
 
                sd.num_spliced += ret;
                sd.total_len -= ret;

diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 185bd8196503d38a83a800b0712d829f494415e6..98c96d649bf90e568a18f409fba18afb232f7b1a 100644 (file)
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -150,6 +150,7 @@ struct netfs_io_stream {
        bool                    active;         /* T if stream is active */
        bool                    need_retry;     /* T if this stream needs retrying */
        bool                    failed;         /* T if this stream failed */
+       bool                    transferred_valid; /* T is ->transferred is valid */
 };
 
 /*