next-20230213 introduced commit d9722a475711 ("splice: Do splice read from
a buffered file without using ITER_PIPE") which broke booting on any
Qualcomm ARM64 device I grabbed, dereferencing a null pointer in
generic_filesplice_read+0xf8/x598. Revert it to make the devices
bootable again.

This reverts commit d9722a47571104f7fa1eeb5ec59044d3607c6070.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 fs/splice.c | 159 +++++++++---------------------------------------------------
 1 file changed, 24 insertions(+), 135 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index fa82dfee1ed0..10b258250868 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -22,7 +22,6 @@
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/pagemap.h>
-#include <linux/pagevec.h>
 #include <linux/splice.h>
 #include <linux/memcontrol.h>
 #include <linux/mm_inline.h>
@@ -378,135 +377,6 @@ static ssize_t generic_file_direct_splice_read(struct file *in, loff_t *ppos,
 	return ret;
 }
 
-/*
- * Splice subpages from a folio into a pipe.
- */
-static size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
-				     struct folio *folio,
-				     loff_t fpos, size_t size)
-{
-	struct page *page;
-	size_t spliced = 0, offset = offset_in_folio(folio, fpos);
-
-	page = folio_page(folio, offset / PAGE_SIZE);
-	size = min(size, folio_size(folio) - offset);
-	offset %= PAGE_SIZE;
-
-	while (spliced < size &&
-	       !pipe_full(pipe->head, pipe->tail, pipe->max_usage)) {
-		struct pipe_buffer *buf = &pipe->bufs[pipe->head & (pipe->ring_size - 1)];
-		size_t part = min_t(size_t, PAGE_SIZE - offset, size - spliced);
-
-		*buf = (struct pipe_buffer) {
-			.ops	= &page_cache_pipe_buf_ops,
-			.page	= page,
-			.offset	= offset,
-			.len	= part,
-		};
-		folio_get(folio);
-		pipe->head++;
-		page++;
-		spliced += part;
-		offset = 0;
-	}
-
-	return spliced;
-}
-
-/*
- * Splice folios from the pagecache of a buffered (ie. non-O_DIRECT) file into
- * a pipe.
- */
-static ssize_t generic_file_buffered_splice_read(struct file *in, loff_t *ppos,
-						 struct pipe_inode_info *pipe,
-						 size_t len,
-						 unsigned int flags)
-{
-	struct folio_batch fbatch;
-	size_t total_spliced = 0, used, npages;
-	loff_t isize, end_offset;
-	bool writably_mapped;
-	int i, error = 0;
-
-	struct kiocb iocb = {
-		.ki_filp	= in,
-		.ki_pos		= *ppos,
-	};
-
-	/* Work out how much data we can actually add into the pipe */
-	used = pipe_occupancy(pipe->head, pipe->tail);
-	npages = max_t(ssize_t, pipe->max_usage - used, 0);
-	len = min_t(size_t, len, npages * PAGE_SIZE);
-
-	folio_batch_init(&fbatch);
-
-	do {
-		cond_resched();
-
-		if (*ppos >= i_size_read(file_inode(in)))
-			break;
-
-		iocb.ki_pos = *ppos;
-		error = filemap_get_pages(&iocb, len, &fbatch, true);
-		if (error < 0)
-			break;
-
-		/*
-		 * i_size must be checked after we know the pages are Uptodate.
-		 *
-		 * Checking i_size after the check allows us to calculate
-		 * the correct value for "nr", which means the zero-filled
-		 * part of the page is not copied back to userspace (unless
-		 * another truncate extends the file - this is desired though).
-		 */
-		isize = i_size_read(file_inode(in));
-		if (unlikely(*ppos >= isize))
-			break;
-		end_offset = min_t(loff_t, isize, *ppos + len);
-
-		/*
-		 * Once we start copying data, we don't want to be touching any
-		 * cachelines that might be contended:
-		 */
-		writably_mapped = mapping_writably_mapped(in->f_mapping);
-
-		for (i = 0; i < folio_batch_count(&fbatch); i++) {
-			struct folio *folio = fbatch.folios[i];
-			size_t n;
-
-			if (folio_pos(folio) >= end_offset)
-				goto out;
-			folio_mark_accessed(folio);
-
-			/*
-			 * If users can be writing to this folio using arbitrary
-			 * virtual addresses, take care of potential aliasing
-			 * before reading the folio on the kernel side.
-			 */
-			if (writably_mapped)
-				flush_dcache_folio(folio);
-
-			n = splice_folio_into_pipe(pipe, folio, *ppos, len);
-			if (!n)
-				goto out;
-			len -= n;
-			total_spliced += n;
-			*ppos += n;
-			in->f_ra.prev_pos = *ppos;
-			if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))
-				goto out;
-		}
-
-		folio_batch_release(&fbatch);
-	} while (len);
-
-out:
-	folio_batch_release(&fbatch);
-	file_accessed(in);
-
-	return total_spliced ? total_spliced : error;
-}
-
 /**
  * generic_file_splice_read - splice data from file to a pipe
  * @in:		file to splice from
@@ -524,13 +394,32 @@ ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
 				 struct pipe_inode_info *pipe, size_t len,
 				 unsigned int flags)
 {
-	if (unlikely(*ppos >= file_inode(in)->i_sb->s_maxbytes))
-		return 0;
-	if (unlikely(!len))
-		return 0;
+	struct iov_iter to;
+	struct kiocb kiocb;
+	int ret;
+
 	if (in->f_flags & O_DIRECT)
 		return generic_file_direct_splice_read(in, ppos, pipe, len, flags);
-	return generic_file_buffered_splice_read(in, ppos, pipe, len, flags);
+
+	iov_iter_pipe(&to, ITER_DEST, pipe, len);
+	init_sync_kiocb(&kiocb, in);
+	kiocb.ki_pos = *ppos;
+	ret = call_read_iter(in, &kiocb, &to);
+	if (ret > 0) {
+		*ppos = kiocb.ki_pos;
+		file_accessed(in);
+	} else if (ret < 0) {
+		/* free what was emitted */
+		pipe_discard_from(pipe, to.start_head);
+		/*
+		 * callers of ->splice_read() expect -EAGAIN on
+		 * "can't put anything in there", rather than -EFAULT.
+		 */
+		if (ret == -EFAULT)
+			ret = -EAGAIN;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(generic_file_splice_read);
 

-- 
2.39.1

Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> next-20230213 introduced commit d9722a475711 ("splice: Do splice read from
> a buffered file without using ITER_PIPE") which broke booting on any
> Qualcomm ARM64 device I grabbed, dereferencing a null pointer in
> generic_filesplice_read+0xf8/x598. Revert it to make the devices
> bootable again.
> 
> This reverts commit d9722a47571104f7fa1eeb5ec59044d3607c6070.
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Commit d9722a47571104f7fa1eeb5ec59044d3607c6070 was part of v13 of my
patches.  This got replaced yesterday by a newer version which may or may not
have made it into linux-next.

This is probably a known bug fixed in the v14 by making shmem have its own
splice-read function.

Can you try this?

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=iov-extract

(Also, can you include me in the cc list as I'm the author of the patch you
reverted?)

Thanks,
David

On 15.02.2023 14:01, David Howells wrote:
> Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> 
>> next-20230213 introduced commit d9722a475711 ("splice: Do splice read from
>> a buffered file without using ITER_PIPE") which broke booting on any
>> Qualcomm ARM64 device I grabbed, dereferencing a null pointer in
>> generic_filesplice_read+0xf8/x598. Revert it to make the devices
>> bootable again.
>>
>> This reverts commit d9722a47571104f7fa1eeb5ec59044d3607c6070.
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Commit d9722a47571104f7fa1eeb5ec59044d3607c6070 was part of v13 of my
> patches.  This got replaced yesterday by a newer version which may or may not
> have made it into linux-next.
> 
> This is probably a known bug fixed in the v14 by making shmem have its own
> splice-read function.
> 
> Can you try this?
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=iov-extract
next-20230216 boots fine again, thanks!

> 
> (Also, can you include me in the cc list as I'm the author of the patch you
> reverted?)
Ugh.. I thought b4 would have done that for me.. weird..

Konrad
> 
> Thanks,
> David
>

On 16.02.23 10:10, Konrad Dybcio wrote:
> 
> 
> On 15.02.2023 14:01, David Howells wrote:
>> Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>> next-20230213 introduced commit d9722a475711 ("splice: Do splice read from
>>> a buffered file without using ITER_PIPE") which broke booting on any
>>> Qualcomm ARM64 device I grabbed, dereferencing a null pointer in
>>> generic_filesplice_read+0xf8/x598. Revert it to make the devices
>>> bootable again.
>>>
>>> This reverts commit d9722a47571104f7fa1eeb5ec59044d3607c6070.
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> Commit d9722a47571104f7fa1eeb5ec59044d3607c6070 was part of v13 of my
>> patches.  This got replaced yesterday by a newer version which may or may not
>> have made it into linux-next.
>>
>> This is probably a known bug fixed in the v14 by making shmem have its own
>> splice-read function.
>>
>> Can you try this?
>>
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=iov-extract
> next-20230216 boots fine again, thanks!
> 
>>
>> (Also, can you include me in the cc list as I'm the author of the patch you
>> reverted?)
> Ugh.. I thought b4 would have done that for me.. weird..

Right, and usually it's nicer to comment on the problematic patches, 
asking for a fix or a revert, instead of sending reverts.

My 2 cents.

-- 
Thanks,

David / dhildenb