On 6/1/24 9:22 AM, Stefan wrote:
> 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.

I don't think that part matters that much.

> Maybe the explicit flag is a bit clearer for users of the API
> compared to the implicit flag when setting sqe->len to zero?

We could go either way. The unused fields returning -EINVAL if set right
now can serve as the flag field - if you have it set, then that is your
length. If not, then the old style is the length. That's the approach I
took, rather than add an explicit flag to it. Existing users that would
set the 64-bit length fields would get -EINVAL already. And since the
normal flags field is already used for advice flags, I'd prefer just
using the existing 64-bit zero fields for it rather than add a flag in
an odd location. Would also make for an easier backport to stable.

But don't feel that strongly about that part.

Attached kernel patch with FEAT added, and liburing patch with 64
versions added.

-- 
Jens Axboe