Re: madvise/fadvise 32-bit length

[Stefan <source@s.muenzel.net>]

On 1/6/2024 17:05, Jens Axboe wrote:
> On 6/1/24 8:19 AM, Jens Axboe wrote:
>> On 6/1/24 3:43 AM, Stefan wrote:
>>> io_uring uses the __u32 len field in order to pass the length to
>>> madvise and fadvise, but these calls use an off_t, which is 64bit on
>>> 64bit platforms.
>>>
>>> When using liburing, the length is silently truncated to 32bits (so
>>> 8GB length would become zero, which has a different meaning of "until
>>> the end of the file" for fadvise).
>>>
>>> If my understanding is correct, we could fix this by introducing new
>>> operations MADVISE64 and FADVISE64, which use the addr3 field instead
>>> of the length field for length.
>>
>> We probably just want to introduce a flag and ensure that older stable
>> kernels check it, and then use a 64-bit field for it when the flag is
>> set.
> 
> I think this should do it on the kernel side, as we already check these
> fields and return -EINVAL as needed. Should also be trivial to backport.
> Totally untested... Might want a FEAT flag for this, or something where
> it's detectable, to make the liburing change straight forward.
> 
> 
> diff --git a/io_uring/advise.c b/io_uring/advise.c
> index 7085804c513c..cb7b881665e5 100644
> --- a/io_uring/advise.c
> +++ b/io_uring/advise.c
> @@ -17,14 +17,14 @@
>   struct io_fadvise {
>   	struct file			*file;
>   	u64				offset;
> -	u32				len;
> +	u64				len;
>   	u32				advice;
>   };
>   
>   struct io_madvise {
>   	struct file			*file;
>   	u64				addr;
> -	u32				len;
> +	u64				len;
>   	u32				advice;
>   };
>   
> @@ -33,11 +33,13 @@ int io_madvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>   #if defined(CONFIG_ADVISE_SYSCALLS) && defined(CONFIG_MMU)
>   	struct io_madvise *ma = io_kiocb_to_cmd(req, struct io_madvise);
>   
> -	if (sqe->buf_index || sqe->off || sqe->splice_fd_in)
> +	if (sqe->buf_index || sqe->splice_fd_in)
>   		return -EINVAL;
>   
>   	ma->addr = READ_ONCE(sqe->addr);
> -	ma->len = READ_ONCE(sqe->len);
> +	ma->len = READ_ONCE(sqe->off);
> +	if (!ma->len)
> +		ma->len = READ_ONCE(sqe->len);
>   	ma->advice = READ_ONCE(sqe->fadvise_advice);
>   	req->flags |= REQ_F_FORCE_ASYNC;
>   	return 0;
> @@ -78,11 +80,13 @@ int io_fadvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>   {
>   	struct io_fadvise *fa = io_kiocb_to_cmd(req, struct io_fadvise);
>   
> -	if (sqe->buf_index || sqe->addr || sqe->splice_fd_in)
> +	if (sqe->buf_index || sqe->splice_fd_in)
>   		return -EINVAL;
>   
>   	fa->offset = READ_ONCE(sqe->off);
> -	fa->len = READ_ONCE(sqe->len);
> +	fa->len = READ_ONCE(sqe->addr);
> +	if (!fa->len)
> +		fa->len = READ_ONCE(sqe->len);
>   	fa->advice = READ_ONCE(sqe->fadvise_advice);
>   	if (io_fadvise_force_async(fa))
>   		req->flags |= REQ_F_FORCE_ASYNC;
> 


If we want to have the length in the same field in both *ADVISE 
operations, we can put a flag in splice_fd_in/optlen.
Maybe the explicit flag is a bit clearer for users of the API
compared to the implicit flag when setting sqe->len to zero?


diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 994bf7af0efe..70794ac1471e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -415,6 +415,14 @@ enum io_uring_msg_ring_flags {
   */
  #define IORING_NOP_INJECT_RESULT	(1U << 0)

+/*
+ * IORING_OP_FADVISE and IORING_OP_MADVISE flags (stored in sqe->optlen)
+ *
+ * IORING_ADVISE_LEN64          Use 64-bit length stored in sqe->addr3
+ *
+ */
+#define IORING_ADVISE_LEN64		(1U << 0)
+
  /*
   * IO completion data structure (Completion Queue Entry)
   */
diff --git a/io_uring/advise.c b/io_uring/advise.c
index 7085804c513c..f229c751adbc 100644
--- a/io_uring/advise.c
+++ b/io_uring/advise.c
@@ -17,14 +17,14 @@
  struct io_fadvise {
  	struct file			*file;
  	u64				offset;
-	u32				len;
+	u64				len;
  	u32				advice;
  };

  struct io_madvise {
  	struct file			*file;
  	u64				addr;
-	u32				len;
+	u64				len;
  	u32				advice;
  };

@@ -32,12 +32,26 @@ int io_madvise_prep(struct io_kiocb *req, const 
struct io_uring_sqe *sqe)
  {
  #if defined(CONFIG_ADVISE_SYSCALLS) && defined(CONFIG_MMU)
  	struct io_madvise *ma = io_kiocb_to_cmd(req, struct io_madvise);
+	u32 flags;

-	if (sqe->buf_index || sqe->off || sqe->splice_fd_in)
+	if (sqe->buf_index || sqe->off)
  		return -EINVAL;

+	flags = READ_ONCE(sqe->optlen);
+
+	if (flags & ~IORING_ADVISE_LEN64)
+		return -EINVAL;
+
+	if (flags & IORING_ADVISE_LEN64) {
+		if (sqe->len)
+			return -EINVAL;
+
+		ma->len = READ_ONCE(sqe->addr3);
+	} else {
+		ma->len = READ_ONCE(sqe->len);
+	}
+
  	ma->addr = READ_ONCE(sqe->addr);
-	ma->len = READ_ONCE(sqe->len);
  	ma->advice = READ_ONCE(sqe->fadvise_advice);
  	req->flags |= REQ_F_FORCE_ASYNC;
  	return 0;
@@ -77,12 +91,26 @@ static bool io_fadvise_force_async(struct io_fadvise 
*fa)
  int io_fadvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
  {
  	struct io_fadvise *fa = io_kiocb_to_cmd(req, struct io_fadvise);
+	u32 flags;

-	if (sqe->buf_index || sqe->addr || sqe->splice_fd_in)
+	if (sqe->buf_index || sqe->addr)
  		return -EINVAL;

+	flags = READ_ONCE(sqe->optlen);
+
+	if (flags & ~IORING_ADVISE_LEN64)
+		return -EINVAL;
+
+	if (flags & IORING_ADVISE_LEN64) {
+		if (sqe->len)
+			return -EINVAL;
+
+		fa->len = READ_ONCE(sqe->addr3);
+	} else {
+		fa->len = READ_ONCE(sqe->len);
+	}
+
  	fa->offset = READ_ONCE(sqe->off);
-	fa->len = READ_ONCE(sqe->len);
  	fa->advice = READ_ONCE(sqe->fadvise_advice);
  	if (io_fadvise_force_async(fa))
  		req->flags |= REQ_F_FORCE_ASYNC;