[2/2] Revert "splice: Do splice read from a buffered file without using ITER_PIPE"
Commit Message
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(-)
Comments
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.
@@ -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);