[PATCH v2] splice: fix premature end of input detection

[PATCH v2] splice: fix premature end of input detection

[Tetsuo Handa]

splice() from pipe should return 0 when there is no pipe writer. However,
since commit a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write()
to preallocate slot") started inserting empty pages, splice() from pipe
also returns 0 when all ready buffers are empty pages.

----------
#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        const int fd = open("/tmp/testfile", O_WRONLY | O_CREAT, 0600);
        int pipe_fd[2] = { -1, -1 };
        pipe(pipe_fd);
        write(pipe_fd[1], NULL, 4096);
	/* This splice() should wait unless interrupted. */
        return !splice(pipe_fd[0], NULL, fd, NULL, 65536, 0);
}
----------

Since such behavior might confuse splice() users, let's fix it by waiting
for non-empty pages inside splice_from_pipe_next().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write() to preallocate slot")
Cc: stable@vger.kernel.org # 5.5+
---
 fs/splice.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/splice.c b/fs/splice.c
index c3d00dfc7344..b58c7ebf6805 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -538,6 +538,8 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
  */
 static int splice_from_pipe_next(struct pipe_inode_info *pipe, struct splice_desc *sd)
 {
+	unsigned int head, tail, mask;
+
 	/*
 	 * Check for signal early to make process killable when there are
 	 * always buffers available
@@ -545,6 +547,7 @@ static int splice_from_pipe_next(struct pipe_inode_info *pipe, struct splice_des
 	if (signal_pending(current))
 		return -ERESTARTSYS;
 
+ refill:
 	while (pipe_empty(pipe->head, pipe->tail)) {
 		if (!pipe->writers)
 			return 0;
@@ -566,7 +569,21 @@ static int splice_from_pipe_next(struct pipe_inode_info *pipe, struct splice_des
 		pipe_wait_readable(pipe);
 	}
 
-	return 1;
+	head = pipe->head;
+	tail = pipe->tail;
+	mask = pipe->ring_size - 1;
+
+	/* dismiss the empty buffers */
+	while (!pipe_empty(head, tail)) {
+		struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+
+		if (likely(buf->len))
+			return 1;
+		pipe_buf_release(pipe, buf);
+		pipe->tail = ++tail;
+	}
+	/* wait again if all buffers were empty */
+	goto refill;
 }
 
 /**
-- 
2.18.4

Re: [PATCH v2] splice: fix premature end of input detection

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

On Mon, Oct 5, 2020 at 5:14 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> splice() from pipe should return 0 when there is no pipe writer. However,
> since commit a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write()
> to preallocate slot") started inserting empty pages, splice() from pipe
> also returns 0 when all ready buffers are empty pages.

Well... Only if you had writers that intentionally did that whole "no
valid data write" thing.

Which could be seen as a feature.

That said, if this actually broke some code, then we need to fix it -
but I really hate how you have that whole !pipe_empty() loop around
the empty buffers.

That case is very unlikely, and you have a loop with !pipe_empty()
*anyway* with the whole "goto refill". So the loop is completely
pointless.

Also, what if we have a packet pipe? Do we perhaps want to return at
packet boundaries? I don't think splice() has cared, so probably not,
but it's worth perhaps thinking about.

Anyway, I'd be a lot happier with the patch being structured something
like this instead.. UNTESTED

                Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 1249 bytes --]

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

diff --git a/fs/splice.c b/fs/splice.c
index c3d00dfc7344..ce75aec52274 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -526,6 +526,22 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
 	return 1;
 }
 
+/* We know we have a pipe buffer, but maybe it's empty? */
+static inline bool eat_empty_buffer(struct pipe_inode_info *pipe)
+{
+	unsigned int tail = pipe->tail;
+	unsigned int mask = pipe->ring_size - 1;
+	struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+
+	if (unlikely(!buf->len)) {
+		pipe_buf_release(pipe, buf);
+		pipe->tail = tail+1;
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * splice_from_pipe_next - wait for some data to splice from
  * @pipe:	pipe to splice from
@@ -545,6 +561,7 @@ static int splice_from_pipe_next(struct pipe_inode_info *pipe, struct splice_des
 	if (signal_pending(current))
 		return -ERESTARTSYS;
 
+repeat:
 	while (pipe_empty(pipe->head, pipe->tail)) {
 		if (!pipe->writers)
 			return 0;
@@ -566,6 +583,9 @@ static int splice_from_pipe_next(struct pipe_inode_info *pipe, struct splice_des
 		pipe_wait_readable(pipe);
 	}
 
+	if (eat_empty_buffer(pipe))
+		goto repeat;
+
 	return 1;
 }
 

Re: [PATCH v2] splice: fix premature end of input detection

[Tetsuo Handa]

On 2020/10/06 3:26, Linus Torvalds wrote:
> On Mon, Oct 5, 2020 at 5:14 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> splice() from pipe should return 0 when there is no pipe writer. However,
>> since commit a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write()
>> to preallocate slot") started inserting empty pages, splice() from pipe
>> also returns 0 when all ready buffers are empty pages.
> 
> Well... Only if you had writers that intentionally did that whole "no
> valid data write" thing.
> 
> Which could be seen as a feature.

pipe_read() is aware of such writers.

  /*
   * We only get here if we didn't actually read anything.
   *
   * However, we could have seen (and removed) a zero-sized
   * pipe buffer, and might have made space in the buffers
   * that way.
   *
   * You can't make zero-sized pipe buffers by doing an empty
   * write (not even in packet mode), but they can happen if
   * the writer gets an EFAULT when trying to fill a buffer
   * that already got allocated and inserted in the buffer
   * array.
   *
   * So we still need to wake up any pending writers in the
   * _very_ unlikely case that the pipe was full, but we got
   * no data.
   */

We also need to care about handle_userfault()
( https://lkml.kernel.org/r/29dd8637-5e44-db4a-9aea-305b079941fb@i-love.sakura.ne.jp )
which we might change it to return an error when pagefault from
pipe_write() takes too long.

> 
> That said, if this actually broke some code, then we need to fix it -
> but I really hate how you have that whole !pipe_empty() loop around
> the empty buffers.
> 
> That case is very unlikely, and you have a loop with !pipe_empty()
> *anyway* with the whole "goto refill". So the loop is completely
> pointless.

This loop just removes all leading empty pages at once.
"goto refill" is a result of all pages were empty.

> 
> Also, what if we have a packet pipe? Do we perhaps want to return at
> packet boundaries? I don't think splice() has cared, so probably not,
> but it's worth perhaps thinking about.

Since manpage says that "Zero-length packets are not supported.", I think that
skipping leading empty pages (either my version or your version) will not break
packet boundaries.

This check is there to avoid returning 0 when all available buffers are empty.
This check should remain no-op if some buffer is not empty.

> 
> Anyway, I'd be a lot happier with the patch being structured something
> like this instead.. UNTESTED

I'm OK with your version.