Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <Y+UJAdnllBw+uxK+@casper.infradead.org>
References: <Y+UJAdnllBw+uxK+@casper.infradead.org> <20230209102954.528942-1-dhowells@redhat.com> <20230209102954.528942-2-dhowells@redhat.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     dhowells@redhat.com, Jens Axboe <axboe@kernel.dk>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christoph Hellwig <hch@infradead.org>, Jan Kara <jack@suse.cz>,
        Jeff Layton <jlayton@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Hillf Danton <hdanton@sina.com>, linux-fsdevel@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org,
        syzbot+a440341a59e3b7142895@syzkaller.appspotmail.com,
        Christoph Hellwig <hch@lst.de>,
        John Hubbard <jhubbard@nvidia.com>
Subject: [PATCH v14 01/12] splice: Fix O_DIRECT file read splice to avoid reversion of ITER_PIPE
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <909201.1675959337.1@warthog.procyon.org.uk>
Content-Transfer-Encoding: quoted-printable
Date:   Thu, 09 Feb 2023 16:15:37 +0000
Message-ID: <909202.1675959337@warthog.procyon.org.uk>
Precedence: bulk

Matthew Wilcox <willy@infradead.org> wrote:

> Please don't use alloc_pages_bulk_list().
...
> If you have that array, you can then use release_pages() ...

Done.  See attached replacement patch.

David
---
splice: Fix O_DIRECT file read splice to avoid reversion of ITER_PIPE

With the upcoming iov_iter_extract_pages() function, pages extracted from =
a
non-user-backed iterator such as ITER_PIPE aren't pinned.
__iomap_dio_rw(), however, calls iov_iter_revert() to shorten the iterator
to just the bufferage it is going to use - which has the side-effect of
freeing the excess pipe buffers, even though they're attached to a bio and
may get written to by DMA (thanks to Hillf Danton for spotting this[1]).

This then causes memory corruption that is particularly noticable when the
syzbot test[2] is run.  The test boils down to:

        out =3D creat(argv[1], 0666);
        ftruncate(out, 0x800);
        lseek(out, 0x200, SEEK_SET);
        in =3D open(argv[1], O_RDONLY | O_DIRECT | O_NOFOLLOW);
        sendfile(out, in, NULL, 0x1dd00);

run repeatedly in parallel.  What I think is happening is that ftruncate()
occasionally shortens the DIO read that's about to be made by sendfile's
splice core by reducing i_size.

Fix this by splitting the handling of a splice from an O_DIRECT file fd of=
f
from that of non-DIO and in this case, replacing the use of an ITER_PIPE
iterator with an ITER_BVEC iterator for which reversion won't free the
buffers.  The DIO-specific code bulk allocates all the buffers it thinks i=
t
is going to use in advance, does the read synchronously and only then trim=
s
the buffer down.  The pages we did use get pushed into the pipe.

This should be more efficient for DIO read by virtue of doing a bulk page
allocation, but slightly less efficient by ignoring any partial page in th=
e
pipe.

Fixes: 920756a3306a ("block: Convert bio_iov_iter_get_pages to use iov_ite=
r_extract_pages")
Reported-by: syzbot+a440341a59e3b7142895@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jens Axboe <axboe@kernel.dk>
cc: Christoph Hellwig <hch@lst.de>
cc: Al Viro <viro@zeniv.linux.org.uk>
cc: David Hildenbrand <david@redhat.com>
cc: John Hubbard <jhubbard@nvidia.com>
cc: linux-mm@kvack.org
cc: linux-block@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20230207094731.1390-1-hdanton@sina.com/ [1=
]
Link: https://lore.kernel.org/r/000000000000b0b3c005f3a09383@google.com/ [=
2]
---
Notes:
    ver #14)
     - Use alloc_pages_bulk_array() rather than alloc_pages_bulk_list().
     - Use release_pages() rather than a loop calling __free_page().
    =

    ver #13)
     - Don't completely replace generic_file_splice_read(), but rather onl=
y use
       this if we're doing a splicing from an O_DIRECT file fd.

 fs/splice.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++=
++++++
 1 file changed, 98 insertions(+)

diff --git a/fs/splice.c b/fs/splice.c
index 5969b7a1d353..91244270b36e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -282,6 +282,101 @@ void splice_shrink_spd(struct splice_pipe_desc *spd)
 	kfree(spd->partial);
 }
 =

+/*
+ * Splice data from an O_DIRECT file into pages and then add them to the =
output
+ * pipe.
+ */
+static ssize_t generic_file_direct_splice_read(struct file *in, loff_t *p=
pos,
+					       struct pipe_inode_info *pipe,
+					       size_t len, unsigned int flags)
+{
+	struct iov_iter to;
+	struct bio_vec *bv;
+	struct kiocb kiocb;
+	struct page **pages;
+	unsigned int head;
+	ssize_t ret;
+	size_t used, npages, chunk, remain, reclaim;
+	int i;
+
+	/* Work out how much data we can actually add into the pipe */
+	used =3D pipe_occupancy(pipe->head, pipe->tail);
+	npages =3D max_t(ssize_t, pipe->max_usage - used, 0);
+	len =3D min_t(size_t, len, npages * PAGE_SIZE);
+	npages =3D DIV_ROUND_UP(len, PAGE_SIZE);
+
+	bv =3D kzalloc(array_size(npages, sizeof(bv[0])) +
+		     array_size(npages, sizeof(struct page *)), GFP_KERNEL);
+	if (!bv)
+		return -ENOMEM;
+
+	pages =3D (void *)(bv + npages);
+	npages =3D alloc_pages_bulk_array(GFP_USER, npages, pages);
+	if (!npages) {
+		kfree(bv);
+		return -ENOMEM;
+	}
+
+	remain =3D len =3D min_t(size_t, len, npages * PAGE_SIZE);
+
+	for (i =3D 0; i < npages; i++) {
+		chunk =3D min_t(size_t, PAGE_SIZE, remain);
+		bv[i].bv_page =3D pages[i];
+		bv[i].bv_offset =3D 0;
+		bv[i].bv_len =3D chunk;
+		remain -=3D chunk;
+	}
+
+	/* Do the I/O */
+	iov_iter_bvec(&to, ITER_DEST, bv, npages, len);
+	init_sync_kiocb(&kiocb, in);
+	kiocb.ki_pos =3D *ppos;
+	ret =3D call_read_iter(in, &kiocb, &to);
+
+	reclaim =3D npages * PAGE_SIZE;
+	remain =3D 0;
+	if (ret > 0) {
+		reclaim -=3D ret;
+		remain =3D ret;
+		*ppos =3D kiocb.ki_pos;
+		file_accessed(in);
+	} else if (ret < 0) {
+		/*
+		 * callers of ->splice_read() expect -EAGAIN on
+		 * "can't put anything in there", rather than -EFAULT.
+		 */
+		if (ret =3D=3D -EFAULT)
+			ret =3D -EAGAIN;
+	}
+
+	/* Free any pages that didn't get touched at all. */
+	reclaim /=3D PAGE_SIZE;
+	if (reclaim) {
+		npages -=3D reclaim;
+		release_pages(pages + npages, reclaim);
+	}
+
+	/* Push the remaining pages into the pipe. */
+	head =3D pipe->head;
+	for (i =3D 0; i < npages; i++) {
+		struct pipe_buffer *buf =3D &pipe->bufs[head & (pipe->ring_size - 1)];
+
+		chunk =3D min_t(size_t, remain, PAGE_SIZE);
+		*buf =3D (struct pipe_buffer) {
+			.ops	=3D &default_pipe_buf_ops,
+			.page	=3D bv[i].bv_page,
+			.offset	=3D 0,
+			.len	=3D chunk,
+		};
+		head++;
+		remain -=3D chunk;
+	}
+	pipe->head =3D head;
+
+	kfree(bv);
+	return ret;
+}
+
 /**
  * generic_file_splice_read - splice data from file to a pipe
  * @in:		file to splice from
@@ -303,6 +398,9 @@ ssize_t generic_file_splice_read(struct file *in, loff=
_t *ppos,
 	struct kiocb kiocb;
 	int ret;
 =

+	if (in->f_flags & O_DIRECT)
+		return generic_file_direct_splice_read(in, ppos, pipe, len, flags);
+
 	iov_iter_pipe(&to, ITER_DEST, pipe, len);
 	init_sync_kiocb(&kiocb, in);
 	kiocb.ki_pos =3D *ppos;