RE: [PATCH 4/4] io_uring/rw: pre-mapped rw attributes

[lizetao <lizetao1@huawei.com>]

Hi,

> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Monday, December 30, 2024 9:30 PM
> To: io-uring@vger.kernel.org
> Cc: asml.silence@gmail.com; Anuj Gupta <anuj20.g@samsung.com>; Kanchan
> Joshi <joshi.k@samsung.com>
> Subject: [PATCH 4/4] io_uring/rw: pre-mapped rw attributes
> 
> Instead of copy_from_user()'ing request attributes, allow it to be grabbwd
> from a registered pre-registered parameter region like we do with registered
> wait arguments.
> 
> Suggested-by: Anuj Gupta <anuj20.g@samsung.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  include/uapi/linux/io_uring.h |  4 +++-
>  io_uring/rw.c                 | 19 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index
> 38f0d6b10eaf..ec6e6fd37d1c 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -112,7 +112,9 @@ struct io_uring_sqe {  };
> 
>  /* sqe->attr_type_mask flags */
> -#define IORING_RW_ATTR_FLAG_PI	(1U << 0)
> +#define IORING_RW_ATTR_FLAG_PI		(1UL << 0)
> +#define IORING_RW_ATTR_REGISTERED	(1UL << 63)
Why use (1UL << 63) instead of (1UL << 1) here?
> +
>  /* PI attribute information */
>  struct io_uring_attr_pi {
>  		__u16	flags;
> diff --git a/io_uring/rw.c b/io_uring/rw.c index dc1acaf95db1..b1db4595788b
> 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -271,10 +271,17 @@ static int io_prep_rw_pi(struct io_kiocb *req, struct
> io_rw *rw, int ddir,
>  	size_t pi_len;
>  	int ret;
> 
> -	if (copy_from_user(&__pi_attr, u64_to_user_ptr(attr_ptr),
> -	    sizeof(pi_attr)))
> -		return -EFAULT;
> -	pi_attr = &__pi_attr;
> +	if (attr_type_mask & IORING_RW_ATTR_REGISTERED) {
> +		pi_attr = io_args_get_ptr(&req->ctx->sqe_args, attr_ptr,
> +					  sizeof(pi_attr));
Here pi_attr is just pointer, so maybe sizeof(__pi_attr) or sizeof(struct io_uring_attr_pi) will be better.
> +		if (IS_ERR(pi_attr))
> +			return PTR_ERR(pi_attr);
> +	} else {
> +		if (copy_from_user(&__pi_attr, u64_to_user_ptr(attr_ptr),
> +		    sizeof(pi_attr)))
Just like above, shoule be sizeof(struct io_uring_attr_pi) here.
> +			return -EFAULT;
> +		pi_attr = &__pi_attr;
> +	}
> 
>  	if (pi_attr->rsvd)
>  		return -EINVAL;
> @@ -294,6 +301,8 @@ static int io_prep_rw_pi(struct io_kiocb *req, struct
> io_rw *rw, int ddir,
>  	return ret;
>  }
> 
> +#define IO_RW_ATTR_ALLOWED_MASK (IORING_RW_ATTR_FLAG_PI |
> +IORING_RW_ATTR_REGISTERED)
> +
>  static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  		      int ddir, bool do_import)
>  {
> @@ -332,7 +341,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct
> io_uring_sqe *sqe,
>  		u64 attr_ptr;
> 
>  		/* only PI attribute is supported currently */
> -		if (attr_type_mask != IORING_RW_ATTR_FLAG_PI)
The comment needs to be adjusted.
> +		if (attr_type_mask & IO_RW_ATTR_ALLOWED_MASK)
Here should be attr_type_mask & ~IO_RW_ATTR_ALLOWED_MASK ?
>  			return -EINVAL;
> 
>  		attr_ptr = READ_ONCE(sqe->attr_ptr);
> --
> 2.47.1
> 

--
Li Zetao