Hi,
> -----Original Message-----
> From: Pavel Begunkov <asml.silence@xxxxxxxxx>
> Sent: Monday, December 30, 2024 9:30 PM
> To: io-uring@xxxxxxxxxxxxxxx
> Cc: asml.silence@xxxxxxxxx; Anuj Gupta <anuj20.g@xxxxxxxxxxx>; Kanchan
> Joshi <joshi.k@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
> ---
> 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
