Re: [PATCH v10 06/10] io_uring: introduce attributes for read/write and PI support

[Pavel Begunkov <asml.silence@gmail.com>]

On 11/27/24 09:46, Anuj Gupta wrote:
> On Tue, Nov 26, 2024 at 03:45:09PM +0000, Pavel Begunkov wrote:
>> On 11/26/24 13:54, Anuj Gupta wrote:
>>> On Tue, Nov 26, 2024 at 01:01:03PM +0000, Pavel Begunkov wrote:
>>>> On 11/25/24 07:06, Anuj Gupta wrote:
>>
>> Hmm, I have doubts it's going to work well because the union
>> members have different sizes. Adding a new type could grow
>> struct io_uring_attr, which is already bad for uapi. And it
>> can't be stacked:
>>
> 
> How about something like this [1]. I have removed the io_uring_attr
> structure, and with the mask scheme the user would pass attributes in
> order of their types. Do you still see some cracks?

Looks good to me

> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
...
> +static int io_prep_rw_pi(struct io_kiocb *req, struct io_rw *rw, int ddir,
> +			 u64 attr_ptr, u64 attr_type_mask)
> +{
> +	struct io_uring_attr_pi pi_attr;
> +	struct io_async_rw *io;
> +	int ret;
> +
> +	if (copy_from_user(&pi_attr, u64_to_user_ptr(attr_ptr),
> +	    sizeof(pi_attr)))
> +		return -EFAULT;
> +
> +	if (pi_attr.rsvd)
> +		return -EINVAL;
> +
> +	io = req->async_data;
> +	io->meta.flags = pi_attr.flags;
> +	io->meta.app_tag = pi_attr.app_tag;
> +	io->meta.seed = READ_ONCE(pi_attr.seed);

Seems an unnecessary READ_ONCE slipped here

> +	ret = import_ubuf(ddir, u64_to_user_ptr(pi_attr.addr),
> +			  pi_attr.len, &io->meta.iter);
> +	if (unlikely(ret < 0))
> +		return ret;
> +	rw->kiocb.ki_flags |= IOCB_HAS_METADATA;
> +	io_meta_save_state(io);
> +	return ret;
> +}
...

-- 
Pavel Begunkov