linux-kernel - Re: [PATCH 3/3] io_uring: add splice(2) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14499431-0409-5d57-9b08-aff95b9d2160@gmail.com>
Date:   Wed, 22 Jan 2020 05:40:10 +0300
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Jens Axboe <axboe@...nel.dk>, io-uring@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Cc:     Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 3/3] io_uring: add splice(2) support

On 22/01/2020 05:03, Jens Axboe wrote:
> On 1/21/20 5:05 PM, Pavel Begunkov wrote:
>> @@ -373,6 +374,15 @@ struct io_rw {
>>  	u64				len;
>>  };
>>  
>> +struct io_splice {
>> +	struct file			*file_in;
>> +	struct file			*file_out;
>> +	loff_t __user			*off_in;
>> +	loff_t __user			*off_out;
>> +	u64				len;
>> +	unsigned int			flags;
>> +};
>> +
>>  struct io_connect {
>>  	struct file			*file;
>>  	struct sockaddr __user		*addr;
> 
> Probably just make that len u32 as per previous email.

Right, I don't want to have multiple types and names for it myself.

> 
>> @@ -719,6 +730,11 @@ static const struct io_op_def io_op_defs[] = {
>>  		.needs_file		= 1,
>>  		.fd_non_neg		= 1,
>>  	},
>> +	[IORING_OP_SPLICE] = {
>> +		.needs_file		= 1,
>> +		.hash_reg_file		= 1,
>> +		.unbound_nonreg_file	= 1,
>> +	}
>>  };
>>  
>>  static void io_wq_submit_work(struct io_wq_work **workptr);
> 
> I probably want to queue up a reservation for the EPOLL_CTL that I
> haven't included yet, but which has been tested. But that's easily
> manageable, so no biggy on my end.

I didn't quite get it. Do you mean collision of opcode numbers?

> 
>> +static bool io_splice_punt(struct file *file)
>> +{
>> +	if (get_pipe_info(file))
>> +		return false;
>> +	if (!io_file_supports_async(file))
>> +		return true;
>> +	return !(file->f_mode & O_NONBLOCK);
>> +}
>> +
>> +static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt,
>> +		     bool force_nonblock)
>> +{
>> +	struct io_splice* sp = &req->splice;
>> +	struct file *in = sp->file_in;
>> +	struct file *out = sp->file_out;
>> +	unsigned int flags = sp->flags;
>> +	long ret;
>> +
>> +	if (force_nonblock) {
>> +		if (io_splice_punt(in) || io_splice_punt(out)) {
>> +			req->flags |= REQ_F_MUST_PUNT;
>> +			return -EAGAIN;
>> +		}
>> +		flags |= SPLICE_F_NONBLOCK;
>> +	}
>> +
>> +	ret = do_splice(in, sp->off_in, out, sp->off_out, sp->len, flags);
>> +	if (force_nonblock && ret == -EAGAIN)
>> +		return -EAGAIN;
>> +
>> +	io_put_file(req->ctx, out, (flags & IOSQE_SPLICE_FIXED_OUT));
>> +	io_cqring_add_event(req, ret);
>> +	if (ret != sp->len)
>> +		req_set_fail_links(req);
>> +	io_put_req_find_next(req, nxt);
>> +	return 0;
>> +}
> 
> This looks good. And this is why the put_file() needs to take separate
> arguments...
> 
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 57d05cc5e271..f234b13e7ed3 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -23,8 +23,14 @@ struct io_uring_sqe {
>>  		__u64	off;	/* offset into file */
>>  		__u64	addr2;
>>  	};
>> -	__u64	addr;		/* pointer to buffer or iovecs */
>> -	__u32	len;		/* buffer size or number of iovecs */
>> +	union {
>> +		__u64	addr;		/* pointer to buffer or iovecs */
>> +		__u64	off_out;
>> +	};
>> +	union {
>> +		__u32	len;	/* buffer size or number of iovecs */
>> +		__s32	fd_out;
>> +	};
>>  	union {
>>  		__kernel_rwf_t	rw_flags;
>>  		__u32		fsync_flags;
>> @@ -37,10 +43,12 @@ struct io_uring_sqe {
>>  		__u32		open_flags;
>>  		__u32		statx_flags;
>>  		__u32		fadvise_advice;
>> +		__u32		splice_flags;
>>  	};
>>  	__u64	user_data;	/* data to be passed back at completion time */
>>  	union {
>>  		__u16	buf_index;	/* index into fixed buffers, if used */
>> +		__u64	splice_len;
>>  		__u64	__pad2[3];
>>  	};
>>  };
> 
> Not a huge fan of this, also mean splice can't ever used fixed buffers.
> Hmm...

But it's not like splice() ever uses user buffers. Isn't it? vmsplice does, but
that's another opcode.

> 
>> @@ -67,6 +75,9 @@ enum {
>>  /* always go async */
>>  #define IOSQE_ASYNC		(1U << IOSQE_ASYNC_BIT)
>>  
>> +/* op custom flags */
>> +#define IOSQE_SPLICE_FIXED_OUT	(1U << 16)
>> +
> 
> I don't think it's unreasonable to say that if you specify
> IOSQE_FIXED_FILE, then both are fixed. If not, then none of them are.
> What do you think?
> 

It's plausible to register only one end for splicing, e.g. splice from
short-lived sockets to pre-registered buffers-pipes. And it's clearer do it now.

-- 
Pavel Begunkov



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)