[RFC] splice/tee: len=0 fast path after validity check

Commit Message

Pavel Begunkov May 9, 2020, 8:46 a.m. UTC

When len=0, splice() and tee() return 0 even if specified fds are
invalid, hiding errors from users. Move len=0 optimisation later after
basic validity checks.

before:
splice(len=0, fd_in=-1, ...) == 0;

after:
splice(len=0, fd_in=-1, ...) == -EBADF;

Signed-off-by: Pavel Begunkov <[e-mail]>
---

Totally leaving it at yours judgment, but it'd be nice to have
for io_uring as well.

 fs/splice.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Jens Axboe May 9, 2020, 2:42 p.m. UTC | #1

On 5/9/20 2:46 AM, Pavel Begunkov wrote:
> When len=0, splice() and tee() return 0 even if specified fds are
> invalid, hiding errors from users. Move len=0 optimisation later after
> basic validity checks.
> 
> before:
> splice(len=0, fd_in=-1, ...) == 0;
> 
> after:
> splice(len=0, fd_in=-1, ...) == -EBADF;

I'm not sure what the purpose of this would be. It probably should have
been done that way from the beginning, but it wasn't.  While there's
very little risk of breaking any applications due to this change, it
also seems like a pointless exercise at this point.

So my suggestion would be to just leave it alone.

Pavel Begunkov May 9, 2020, 4:03 p.m. UTC | #2

On 09/05/2020 17:42, Jens Axboe wrote:
> On 5/9/20 2:46 AM, Pavel Begunkov wrote:
>> When len=0, splice() and tee() return 0 even if specified fds are
>> invalid, hiding errors from users. Move len=0 optimisation later after
>> basic validity checks.
>>
>> before:
>> splice(len=0, fd_in=-1, ...) == 0;
>>
>> after:
>> splice(len=0, fd_in=-1, ...) == -EBADF;
> 
> I'm not sure what the purpose of this would be. It probably should have
> been done that way from the beginning, but it wasn't.  While there's
> very little risk of breaking any applications due to this change, it
> also seems like a pointless exercise at this point.
> 
> So my suggestion would be to just leave it alone.

Ok

Patch

diff --git a/fs/splice.c b/fs/splice.c
index a1dd54de24d8..8d6fc690f8e9 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1122,6 +1122,9 @@  long do_splice(struct file *in, loff_t __user *off_in,
 		     !(out->f_mode & FMODE_WRITE)))
 		return -EBADF;
 
+	if (unlikely(!len))
+		return 0;
+
 	ipipe = get_pipe_info(in);
 	opipe = get_pipe_info(out);
 
@@ -1426,9 +1429,6 @@  SYSCALL_DEFINE6(splice, int, fd_in, loff_t __user *, off_in,
 	struct fd in, out;
 	long error;
 
-	if (unlikely(!len))
-		return 0;
-
 	if (unlikely(flags & ~SPLICE_F_ALL))
 		return -EINVAL;
 
@@ -1535,7 +1535,6 @@  static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,
 	int ret = 0;
 	bool input_wakeup = false;
 
-
 retry:
 	ret = ipipe_prep(ipipe, flags);
 	if (ret)
@@ -1769,6 +1768,9 @@  long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags)
 	 * copying the data.
 	 */
 	if (ipipe && opipe && ipipe != opipe) {
+		if (unlikely(!len))
+			return 0;
+
 		if ((in->f_flags | out->f_flags) & O_NONBLOCK)
 			flags |= SPLICE_F_NONBLOCK;
 
@@ -1795,9 +1797,6 @@  SYSCALL_DEFINE4(tee, int, fdin, int, fdout, size_t, len, unsigned int, flags)
 	if (unlikely(flags & ~SPLICE_F_ALL))
 		return -EINVAL;
 
-	if (unlikely(!len))
-		return 0;
-
 	error = -EBADF;
 	in = fdget(fdin);
 	if (in.file) {